FixIt improvements #19
Conversation
WalkthroughConfiguration updates to SwiftLint and tests; multiple public API additions and signatures changes across macro syntax code (FixIt helpers, GlobalActorIsolation, access-control utilities), parser signature/simplifications, flattened member helpers, and corresponding test updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
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 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Sources/PrincipleMacros/Diagnostics/FixIt.swift (1)
13-22: Consider preserving trailing trivia when removing nodes.The current implementation only preserves
leadingTriviawhen removing a node, which may result in formatting issues. If the removed node has trailing whitespace or newlines, they will be lost, potentially collapsing adjacent tokens or lines.Consider whether trailing trivia should also be preserved:
public static func remove( message: String, oldNode: some SyntaxProtocol ) -> Self { .replace( message: MacroExpansionFixItMessage(message), oldNode: oldNode, - newNode: "\(oldNode.leadingTrivia)" as TokenSyntax + newNode: "\(oldNode.leadingTrivia)\(oldNode.trailingTrivia)" as TokenSyntax ) }Alternatively, if leading-trivia-only is intentional for your use cases, consider adding a doc comment explaining this behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.swiftlint.tests.yml(1 hunks).swiftlint.yml(6 hunks)Sources/PrincipleMacros/Diagnostics/FixIt.swift(1 hunks)Sources/PrincipleMacros/Parameters/ParameterExtractor.swift(1 hunks)Sources/PrincipleMacros/Parsers/EnumCases/EnumCasesParser.swift(1 hunks)Sources/PrincipleMacros/Parsers/Properties/PropertiesParser.swift(1 hunks)Sources/PrincipleMacros/Syntax/Concepts/GlobalActorIsolation.swift(1 hunks)Sources/PrincipleMacros/Syntax/Extensions/AttributedTypeSyntax.swift(1 hunks)Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax+AccessControlLevel.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax+AccessControlLevel.swift (2)
Sources/PrincipleMacros/Parsers/Common/ParserResultsCollection.swift (1)
filter(40-44)Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
accessControlLevel(83-112)
Sources/PrincipleMacros/Diagnostics/FixIt.swift (1)
Sources/PrincipleMacros/Syntax/Extensions/SyntaxProtocol.swift (1)
withTrivia(37-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (visionos)
- GitHub Check: build-and-test (tvos)
- GitHub Check: build-and-test (macos)
- GitHub Check: build-and-test (maccatalyst)
🔇 Additional comments (12)
Sources/PrincipleMacros/Parsers/EnumCases/EnumCasesParser.swift (1)
16-18: LGTM!Using the explicit
EnumCasesList()initializer instead of.init()improves code clarity by making the return type immediately visible at the call site.Sources/PrincipleMacros/Parsers/Properties/PropertiesParser.swift (1)
16-18: LGTM!Consistent with the
EnumCasesParserchange—using explicitPropertiesList()improves readability..swiftlint.tests.yml (1)
1-4: LGTM!Disabling
type_body_lengthfor tests is appropriate—test classes often grow large due to numerous test methods, and this rule would generate noise without actionable benefit.Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax+AccessControlLevel.swift (1)
26-41: LGTM!The
withAccessControlLevelmethod correctly:
- Filters out existing access control modifiers (both getter and setter variants)
- Optionally prepends a new access control modifier when
levelis non-nil- Follows Swift's
withnaming convention for copy-returning transformationsClean and useful helper for macro-based code generation.
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
189-191: LGTM!Simplified return for the
nilliteral case—returning.nonisolateddirectly aligns with the updatedGlobalActorIsolationAPI where the nonisolated variant no longer requires an associated modifier when created from a nil input.Sources/PrincipleMacros/Syntax/Concepts/GlobalActorIsolation.swift (1)
16-19: LGTM!The factory method provides a clean, convenient way to create the nonisolated case without requiring callers to manually construct the
DeclModifierSyntax. This improves the API ergonomics.Sources/PrincipleMacros/Diagnostics/FixIt.swift (1)
24-34: LGTM!The helper correctly preserves trivia from the old node when replacing it with the new node using
withTrivia(from:), which ensures proper formatting is maintained..swiftlint.yml (4)
11-11: LGTM!The comment updates clearly indicate which rules are now handled by swiftformat, improving maintainability by documenting the tooling split.
Also applies to: 60-60, 70-70, 147-147
176-176: LGTM!Adding
ignores_multiline_strings: trueis a sensible configuration, as breaking multiline string literals to meet line length requirements can alter their content or readability.
204-204: LGTM!The updated regex pattern correctly excludes empty type bodies using
(?!\\s*\\})while enforcing the empty line requirement after type declarations. The pattern handles the various type keywords appropriately.
58-58: Enablingimplicitly_unwrapped_optionalis safe; no actual code violations exist.The codebase contains no implicitly unwrapped optional type declarations in actual code. The only instances of
!in type contexts appear in test string fixtures (TypeSyntaxTests.swift and ClosureTypeSyntaxTests.swift), which are string representations and not executable code that would trigger the rule.Sources/PrincipleMacros/Syntax/Extensions/AttributedTypeSyntax.swift (1)
13-16: This is a new initializer addition with no breaking changes; no action needed.No existing callers use this two-parameter initializer. The method is a new convenience constructor that does not affect the default
AttributedTypeSyntax(syntax:)initializer from SwiftSyntax, which is what existing code relies on. The type parameter is specific to this new initializer and poses no compatibility risk.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Sources/PrincipleMacros/Syntax/Concepts/GlobalActorIsolation.swift (2)
13-19: Staticnonisolatedfactory wiring looks correct; consider future‑proofing the label spellingThe new
public static var nonisolatedcorrectly constructs the.nonisolatedcase with aDeclModifierSyntaxfor thenonisolatedkeyword, and integrates cleanly withtrimmedNonisolatedModifier.One small API‑surface nit: the associated value label is spelled
trimmedModifer(missing the second “i”). If you expect external code to construct.nonisolated(trimmedModifer:)directly, this typo becomes harder to fix later without a breaking change. If you can still afford it in this release, consider renaming totrimmedModifier(and updating the call here) or introducing an additional correctly spelled factory while deprecating the old spelling.
73-75: Generalizing_resolvedtosome Sequence<Syntax>is a sound internal improvementAllowing
_resolvedto acceptsome Sequence<Syntax>makes the helper more flexible and lets call sites pass lightweight sequences likeCollectionOfOnewithout first materializing an array, while preserving the existingfor‑loop behavior.As a minor improvement, you might add a brief doc comment noting the expected ordering of
fullContext(e.g., starting from the innermost declaration outward) so future call sites constructing custom sequences don’t accidentally violate the early‑breakassumptions aroundDeclGroupSyntax.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Sources/PrincipleMacros/Parsers/Common/Parser.swift(1 hunks)Sources/PrincipleMacros/Parsers/Properties/PropertiesParser.swift(2 hunks)Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift(1 hunks)Sources/PrincipleMacros/Syntax/Concepts/GlobalActorIsolation.swift(2 hunks)Sources/PrincipleMacros/Syntax/Extensions/AttributedTypeSyntax.swift(2 hunks)Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift(4 hunks)Sources/PrincipleMacros/Syntax/Extensions/MemberBlockItemListSyntax.swift(1 hunks)Tests/PrincipleMacrosTests/Parsers/PropertiesListTests.swift(1 hunks)Tests/PrincipleMacrosTests/Syntax/Concepts/AccessControlLevelTests.swift(1 hunks)Tests/PrincipleMacrosTests/Syntax/Extensions/IfConfigDeclSyntaxTests.swift(4 hunks)Tests/PrincipleMacrosTests/Syntax/Extensions/MemberBlockItemListSyntaxTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/PrincipleMacros/Syntax/Extensions/AttributedTypeSyntax.swift
🧰 Additional context used
🧬 Code graph analysis (6)
Tests/PrincipleMacrosTests/Parsers/PropertiesListTests.swift (3)
Sources/PrincipleMacros/Parsers/Common/Parser.swift (2)
parse(22-27)parse(29-37)Sources/PrincipleMacros/Parsers/Properties/PropertiesParser.swift (1)
parse(13-44)Sources/PrincipleMacros/Parsers/EnumCases/EnumCasesParser.swift (1)
parse(13-25)
Sources/PrincipleMacros/Syntax/Extensions/MemberBlockItemListSyntax.swift (1)
Tests/PrincipleMacrosTests/Syntax/Extensions/MemberBlockItemListSyntaxTests.swift (2)
ifConfig(28-42)members(16-26)
Sources/PrincipleMacros/Parsers/Properties/PropertiesParser.swift (3)
Sources/PrincipleMacros/Parsers/Common/Parser.swift (2)
parse(22-27)parse(29-37)Sources/PrincipleMacros/Parsers/EnumCases/EnumCasesParser.swift (1)
parse(13-25)Sources/PrincipleMacros/Syntax/Extensions/TypeSyntax.swift (1)
first(31-35)
Sources/PrincipleMacros/Parsers/Common/Parser.swift (1)
Tests/PrincipleMacrosTests/Syntax/Extensions/MemberBlockItemListSyntaxTests.swift (1)
members(16-26)
Tests/PrincipleMacrosTests/Syntax/Concepts/AccessControlLevelTests.swift (1)
Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift (3)
forMember(111-122)forSibling(124-135)forPeer(137-148)
Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift (1)
Tests/PrincipleMacrosTests/Syntax/Extensions/MemberBlockItemListSyntaxTests.swift (1)
ifConfig(28-42)
🪛 SwiftLint (0.57.0)
Tests/PrincipleMacrosTests/Syntax/Extensions/MemberBlockItemListSyntaxTests.swift
[Warning] 41-41: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 61-61: Magic numbers should be replaced by named constants
(no_magic_numbers)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-test (visionos)
- GitHub Check: build-and-test (tvos)
- GitHub Check: build-and-test (macos)
- GitHub Check: build-and-test (ios)
- GitHub Check: build-and-test (maccatalyst)
🔇 Additional comments (24)
Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift (4)
9-9: LGTM: Import updated to SwiftSyntaxMacros.The import change aligns with the macro-focused extensions in this file.
69-78: LGTM: Clean API for conditional IfConfig application.The
withIfConfigIfPresentmethod provides an intuitive interface for applying enclosing IfConfig when available, falling back to the original list when not.
97-109: LGTM: Consistent API extension for CodeBlockItemListSyntax.The implementation mirrors
MemberBlockItemListSyntax.withIfConfigIfPresent, appropriately using.statements(self)instead of.decls(self).
120-124: LGTM: Reduced visibility and improved formatting.The method is now appropriately scoped as
fileprivatesince it's an internal helper, and the addition ofwithLeadingNewlineensures consistent formatting of the resulting IfConfig.Tests/PrincipleMacrosTests/Parsers/PropertiesListTests.swift (1)
37-37: LGTM: Test updated to use new parser API.The test correctly switches from
parse(memberBlock:)toparse(declarationGroup:), aligning with the parser API refactoring.Tests/PrincipleMacrosTests/Syntax/Extensions/MemberBlockItemListSyntaxTests.swift (1)
12-64: LGTM: Comprehensive test coverage for flattening.The tests appropriately cover:
- Basic member flattening (no IfConfig)
- Single-level IfConfig flattening
- Nested IfConfig flattening
The magic number warnings from SwiftLint are acceptable in test assertions.
Tests/PrincipleMacrosTests/Syntax/Extensions/IfConfigDeclSyntaxTests.swift (3)
18-18: LGTM: Test updated to use new parser API.Correctly switches to
parse(declarationGroup:)in line with the parser refactoring.
133-164: LGTM: Test renamed and updated for new API.The test is appropriately renamed to
applyToMemberBlockand correctly useswithIfConfigIfPresent(from:). The removal of optional chaining on.descriptionat line 163 is correct sincewithIfConfigIfPresentalways returns a non-optional value.
167-198: LGTM: Test renamed and updated for new API.The test is appropriately renamed to
applyToCodeBlockand correctly useswithIfConfigIfPresent(from:). The removal of optional chaining on.descriptionat line 197 is correct sincewithIfConfigIfPresentalways returns a non-optional value.Sources/PrincipleMacros/Parsers/Properties/PropertiesParser.swift (2)
17-17: LGTM: Explicit constructor improves clarity.Using
PropertiesList()is more explicit and readable than.init().
46-62: LGTM: Clean single-binding validation API.The
parseStandalonemethod provides a clear interface for parsing declarations that must contain exactly one binding. The validation logic correctly:
- Returns
nilfor empty results- Throws a descriptive error for multiple bindings
- Returns the single property when exactly one exists
Sources/PrincipleMacros/Parsers/Common/Parser.swift (2)
22-27: LGTM: Simplified parser entry point.The refactored
parse(declarationGroup:)method delegates IfConfig flattening to theflattenedproperty, improving separation of concerns. The parser now focuses purely on parsing logic rather than structural traversal.
29-37: LGTM: Generalized member parsing.The signature now accepts
some Sequence<MemberBlockItemSyntax>instead of a concreteMemberBlockItemListSyntax, making it more flexible. The removal of IfConfig-specific handling is appropriate since flattening is now handled upstream.Sources/PrincipleMacros/Syntax/Extensions/MemberBlockItemListSyntax.swift (3)
11-22: LGTM: Efficient recursive flattening implementation.The
flattenedproperty correctly:
- Uses lazy evaluation to defer computation
- Recursively flattens
IfConfigDeclSyntaxmembers- Wraps non-IfConfig members in
CollectionOfOne- Uses
AnySequencefor appropriate type erasure
24-29: LGTM: Clean flattening across IfConfig clauses.The implementation efficiently concatenates flattened members from all clauses using lazy evaluation.
31-41: LGTM: Appropriate handling of clause elements.The implementation correctly:
- Recursively flattens
.declscase members- Returns an empty collection for non-
.declscases (e.g.,.statements)Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift (1)
9-9: LGTM! Appropriate Sendable conformance.Adding
Sendableconformance to this enum is safe and beneficial. The enum is a value type with only cases and computed properties, making it inherently thread-safe for use across concurrency boundaries.Tests/PrincipleMacrosTests/Syntax/Concepts/AccessControlLevelTests.swift (6)
9-12: LGTM! Clean test setup.The imports and test struct declaration are properly configured for the Swift Testing framework.
14-28: LGTM! Comprehensive round-trip conversion test.The parameterized test correctly validates that all access control keywords can be converted to
AccessControlLeveland back, ensuring the conversion logic is bidirectional.
30-37: LGTM! Correct access control ordering.The comparison test validates the expected hierarchy of access control levels, which aligns with the enum's
Intraw value ordering.
40-70: LGTM! Thorough member inheritance coverage.The test suite correctly validates the
forMemberbehavior:
- Private members cannot inherit (return
nil)- Open defaults to public for members
- Other levels are preserved
The use of
dropFirst().dropLast()appropriately excludes the edge cases (private and open) that are tested separately.
72-102: LGTM! Accurate sibling inheritance tests.The sibling inheritance tests correctly validate:
- Private becomes fileprivate for siblings
- Open defaults to public
- Other levels are preserved
104-127: LGTM! Correct peer inheritance tests.The peer inheritance tests appropriately validate that:
- Open defaults to public for peers
- All other levels (including private) are preserved
The use of
dropLast()(rather thandropFirst().dropLast()) is correct because private peers should retain their access level.Sources/PrincipleMacros/Syntax/Concepts/GlobalActorIsolation.swift (1)
22-23: Extension boundary and placement are fineKeeping the computed accessors in a dedicated
extension GlobalActorIsolationkeeps the core enum definition compact and is consistent with the rest of the file’s structure. No issues here.
Summary by CodeRabbit
Chores
Refactor
New Public APIs
Tests
✏️ Tip: You can customize this high-level summary in your review settings.