Skip to content

Feature/project#28

Merged
swiftty merged 8 commits into
mainfrom
feature/project
May 12, 2026
Merged

Feature/project#28
swiftty merged 8 commits into
mainfrom
feature/project

Conversation

@swiftty
Copy link
Copy Markdown
Owner

@swiftty swiftty commented May 12, 2026

No description provided.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request integrates swift-format for consistent code styling and updates the package manifest to apply global Swift settings and linting across targets. Key changes include adopting public import for several modules, transitioning to any Error in Task types, and refactoring LRUCache tests to use a unified protocol for cross-implementation testing. Feedback focuses on simplifying the target configuration logic in Package.swift, correcting typos in method names and log strings, and replacing non-idiomatic if case let usage with standard assignments.

Comment thread Package.swift
Comment on lines +44 to +70
for target in package.targets {
if [.executable, .test, .regular].contains(target.type) {
do {
var swiftSettings = target.swiftSettings ?? []
defer {
target.swiftSettings = swiftSettings
}
swiftSettings += [
.enableUpcomingFeature("InternalImportsByDefault"),
.enableUpcomingFeature("NonisolatedNonsendingByDefault"),
.enableUpcomingFeature("MemberImportVisibility"),
.enableUpcomingFeature("InferIsolatedConformances"),
.enableUpcomingFeature("ImmutableWeakCaptures"),
.enableUpcomingFeature("ExistentialAny")
]
}
do {
var plugins = target.plugins ?? []
defer {
target.plugins = plugins
}
plugins += [
.plugin(name: "Lint", package: "swift-format-plugin")
]
}
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The loop to apply global settings can be simplified by removing the do blocks and defer statements, which makes the configuration more concise and readable.

for target in package.targets {
    if [.executable, .test, .regular].contains(target.type) {
        target.swiftSettings = (target.swiftSettings ?? []) + [
            .enableUpcomingFeature("InternalImportsByDefault"),
            .enableUpcomingFeature("NonisolatedNonsendingByDefault"),
            .enableUpcomingFeature("MemberImportVisibility"),
            .enableUpcomingFeature("InferIsolatedConformances"),
            .enableUpcomingFeature("ImmutableWeakCaptures"),
            .enableUpcomingFeature("ExistentialAny")
        ]
        target.plugins = (target.plugins ?? []) + [
            .plugin(name: "Lint", package: "swift-format-plugin")
        ]
    }
}

}

private func peformChange(_ change: Staging<Key>.Change, with url: URL) -> Task<Void, Error> {
private func peformChange(_ change: Staging<Key>.Change, with url: URL) -> Task<Void, any Error> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Typo in method name: peformChange should be performChange. Note that you should also update the call site in the _flush method.

Suggested change
private func peformChange(_ change: Staging<Key>.Change, with url: URL) -> Task<Void, any Error> {
private func performChange(_ change: Staging<Key>.Change, with url: URL) -> Task<Void, any Error> {

Comment on lines 300 to +302
if case let dir = url.deletingLastPathComponent(),
!FileManager.default.fileExists(atPath: dir.path) {
!FileManager.default.fileExists(atPath: dir.path)
{
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using if case let for a non-optional assignment is non-idiomatic in Swift. It is clearer and more readable to perform the assignment before the if statement.

                    let dir = url.deletingLastPathComponent()
                    if !FileManager.default.fileExists(atPath: dir.path) {

size -= item.meta.totalFileAllocatedSize ?? 0
logger.debug("\(self.logKey)sweeped item: \(item.url.lastPathComponent), size: \(item.meta.totalFileAllocatedSize ?? 0)")
logger.debug(
"\(self.logKey)sweeped item: \(item.url.lastPathComponent), size: \(item.meta.totalFileAllocatedSize ?? 0)"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Typo in log message: sweeped should be swept.

Suggested change
"\(self.logKey)sweeped item: \(item.url.lastPathComponent), size: \(item.meta.totalFileAllocatedSize ?? 0)"
"\(self.logKey)swept item: \(item.url.lastPathComponent), size: \(item.meta.totalFileAllocatedSize ?? 0)"

@swiftty swiftty force-pushed the feature/project branch from 6053558 to 02940a4 Compare May 12, 2026 15:22
@swiftty swiftty merged commit 104618c into main May 12, 2026
1 check passed
@swiftty swiftty deleted the feature/project branch May 12, 2026 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant