Sync workflows + lint configs from MistKit; fix all warnings#159
Conversation
… warnings Adopt MistKit's CI infrastructure and stricter lint rules; migrate source and tests off deprecated SwiftSyntax/SyntaxKit APIs so the build is warning- free across swift build, swift-format, swiftlint, and periphery. Closes #126, #127, #128, #136, #138, #143, #144. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.0.5 #159 +/- ##
==========================================
- Coverage 78.15% 78.04% -0.12%
==========================================
Files 126 127 +1
Lines 4555 4491 -64
==========================================
- Hits 3560 3505 -55
+ Misses 995 986 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code Review: PR #159 — Sync workflows + lint configs from MistKit; fix all warningsSummaryThis PR syncs CI infrastructure and lint configuration from the MistKit companion project and eliminates all build-time warnings surfaced by now-stricter lint rules. Changes fall into five categories: new GitHub Actions workflows ( Code Quality and Best Practices ✅Positive:
Issues:
Potential Bugs 🐛
Performance Considerations ⚡
Security Considerations 🔒
Test CoverageNo new unit tests are needed for this infrastructure PR. The new workflows themselves serve as CI-level tests. The deprecated-API fixes are validated by the existing 397-test suite continuing to pass. Actionable Suggestions
Overall: Solid infrastructure and warning-cleanup PR. The SwiftSyntax API migration fixes are correct and necessary. The main items to address before merge are pinning 🤖 Generated with Claude Code |
| let memberAccess = MemberAccessExprSyntax( | ||
| base: typeName.isEmpty | ||
| ? nil : ExprSyntax(DeclReferenceExprSyntax(baseName: .identifier(typeName))), | ||
| dot: .periodToken(), |
There was a problem hiding this comment.
Two things bundled in this hunk:
MemberAccessExprSyntax(base:dot:name:)is deprecated in current SwiftSyntax; replaced byMemberAccessExprSyntax(base:period:declName: DeclReferenceExprSyntax(...)). That clears a deprecation warning the stricter build surfaced.patternWithTupleandtuplePatternwere dead code — assigned but never read. The catch pattern is actually built right below viapatternSyntaxExpr+FunctionCallExprSyntax+ nestedLabeledExprSyntax(lines ~58–88 in the new file). The compiler was emitting "Initialization of immutable value … was never used" for both. Removing them eliminates two warnings; the produced syntax is unchanged.
| case let .asyncThrows(isRethrows, errorType): | ||
| case .asyncThrows(let isRethrows, let errorType): | ||
| let throwsSpecifier = buildThrowsSpecifier(isRethrows: isRethrows) | ||
| if let errorType = errorType { |
There was a problem hiding this comment.
Three cleanups in this hunk:
FunctionEffectSpecifiersSyntax(asyncSpecifier:throwsSpecifier:)is deprecated; switched every branch to thethrowsClause:variant.- Once both sides go through
throwsClause, theif errorType != nilsplit inside eachcaseis redundant. WidenedbuildThrowsClause(throwsSpecifier:errorType:)to acceptString?and pushed the with/without typed-throws-parens logic into the helper (lines 63–77 of the new file). Eachcasearm now collapses to onereturn FunctionEffectSpecifiersSyntax(...). case let .throws(isRethrows, errorType)→case .throws(let isRethrows, let errorType)to satisfy swift-format's newly-enabledUseLetInEveryBoundCaseVariablerule. Same semantics, style only.
- Bump Xcode 26.4 → 26.5 and osVersion 26.4(.1) → 26.5 across SyntaxKit.yml and codeql.yml on macos-26 matrix entries (leaves macos-15 + Xcode_16.x back-compat checks intact). - Add swift:6.0 to swift-source-compat.yml so the suite runs against every supported Swift toolchain. - Split ProcessError into its own file in DocumentationHarness/ and Pattern out of LetBindingPattern.swift. Removes the // swiftlint:disable one_declaration_per_file block-disables so the rule is enforced. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review: PR #159 — Sync workflows + lint configs; fix all warningsOverviewThis PR synchronizes CI workflows, lint configs, and SwiftSyntax API migrations from a sibling project (MistKit). It's a maintenance/housekeeping PR that fixes deprecation warnings across 40+ source files and tightens code quality tooling. ✅ What's working wellCI / Workflow improvements
SwiftSyntax API migrations
Lint config tightening
|
The MistKit sync dropped --strict from SWIFTFORMAT_OPTIONS in the STRICT branch, leaving swiftlint --strict but no equivalent for swift-format. --strict is a lint-only flag (the format subcommand rejects it with "Unknown option"), so split it into its own SWIFTFORMAT_LINT_STRICT variable that's only applied to the `swift-format lint` invocation. Also drops the duplicated --configuration .swift-format that the previous shared-options layout caused on the lint line. Local default and STRICT modes both pass; 397 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks for the second pass. Round-2 responses: A. B. C.
I went with inline because each annotation carries its own tail-comment justification ( D. CI checklist — checking now after the new push lands; will tick the boxes on the PR description once the new commits' check-unsafe-flags / swift-source-compat / cache-hit runs report green. |
Code Review — PR #159: Sync workflows + lint configs from MistKit; fix all warningsOverviewThis PR is a well-scoped housekeeping change: sync CI workflows and lint configs from MistKit, tighten lint rules, and fix every warning they surface. The mechanical warning fixes (deprecated SwiftSyntax inits, unused public imports, periphery annotations) are all correct. CI improvements are meaningful. A few things worth noting: Strengths
Concerns1. SwiftLint version mismatch
2. container:
image: swift:latest
3. pushd $PACKAGE_DIR # ← unquotedThis is pre-existing (the prior line used 4. The rule was removed from the opt-in list without a comment or mention in the PR description. Was this intentional (conflicts with another rule? noisy?) or an accidental drop during the sync? Worth a one-line note. Minor notes
SummaryThe CI improvements are solid and the warning fixes are mechanical/correct. Before merging, I'd address: (1) the SwiftLint version discrepancy between |
Scripts/lint.sh now invokes header.sh against Tests as well as Sources, so copyright years stay in sync (refreshed 2025 → 2026 across Tests). SwiftLint invocations now take explicit Sources Tests paths so they never traverse .build*. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
.swift-format,.swiftlint.yml,Scripts/lint.sh,mise.toml, and GitHub workflows with MistKit's current state. Adds.github/actions/setup-tools/action.yml,check-unsafe-flags.yml, andswift-source-compat.yml; refactorsSyntaxKit.ymlmatrix andcodeql.ymlSwift-on-macOS gating.NeverForceUnwrap,NeverUseForceTry,NeverUseImplicitlyUnwrappedOptionals,UseLetInEveryBoundCaseVariable,ValidateDocumentationComments) and SwiftLint additions (discouraged_optional_boolean,one_declaration_per_file,no_unchecked_sendablecustom rule,type_namelength config).TupleExprElementListSyntax/ForInStmtSyntax/RepeatWhileStmtSyntax/MemberAccessExprSyntax(name:)/DictionaryElementSyntax(keyExpression:)/FunctionEffectSpecifiersSyntax(throwsSpecifier:)/etc.), unused-public-import demotions, periphery dead-code annotations,#warningTODOs converted to plain comments, deprecated SyntaxKit test API migrations (PlusAssign→Infix("+="),Parameter(isUnnamed:)→Parameter(unlabeled:),Infix(_:_:)builder closure →Infix(_:lhs:rhs:)), and removal of unnecessarytrycascades.Test plan
swift build --build-tests— 0 warnings, 0 errorsswift test— 397 tests / 74 suites passswift-format lint— cleanswiftlint lint— cleanperiphery scan— cleancheck-unsafe-flagsandswift-source-compatworkflows passlintjob uses the cachedsetup-toolsaction (cache hit on rerun)v0.0.5🤖 Generated with Claude Code