Added setterAccessControlLevel
#11
Conversation
|
""" WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Property as Property Declaration
participant WithModifiers as WithModifiersSyntax
participant Modifiers as Modifiers List
Property->>WithModifiers: Query accessControlLevel
WithModifiers->>WithModifiers: accessControlLevel(detail: nil)
WithModifiers->>Modifiers: Filter modifiers for access control
Modifiers-->>WithModifiers: Return first matching token
WithModifiers-->>Property: Return accessControlLevel
Property->>WithModifiers: Query setterAccessControlLevel
WithModifiers->>WithModifiers: accessControlLevel(detail: "set")
WithModifiers->>Modifiers: Filter modifiers for 'set' access control
Modifiers-->>WithModifiers: Return first matching token or nil
WithModifiers-->>Property: Return setterAccessControlLevel or fallback to accessControlLevel
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Tests/PrincipleMacrosTests/Parsers/PropertiesParserTests.swift (2)
42-44: Potential confusion about access control level interpretationThe test correctly verifies that
accessControlLevelis nil forprivate(set), but this behavior might be surprising to users who would expect an implicit "internal" access level. Consider adding a comment to clarify this behavior.#expect(property.accessControlLevel == nil) #expect(property.setterAccessControlLevel?.trimmedDescription == "private") +// Note: accessControlLevel is nil because the declaration only specifies +// an access level for the setter, not for the property itself
36-110: Suggest adding a test case for combined access control modifiersThe tests cover various scenarios but don't include a case for properties with both general and setter-specific access control modifiers (e.g.,
public private(set) var). This would provide more comprehensive coverage.@Test +func testStoredVarWithCombinedAccessControl() throws { + let decl: DeclSyntax = """ + public private(set) var myVar = "Combined access control" + """ + let property = try #require(PropertiesParser.parse(declaration: decl, in: context).first) + #expect(property.kind == .stored) + #expect(property.mutability == .mutable) + #expect(property.accessControlLevel?.trimmedDescription == "public") + #expect(property.setterAccessControlLevel?.trimmedDescription == "private") + #expect(property.typeScopeSpecifier == nil) + #expect(property.trimmedName.description == "myVar") + #expect(property.inferredType.description == "String") + #expect(property.accessors == nil) + #expect(property.observers == nil) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax.swift(1 hunks)Tests/PrincipleMacrosTests/Parsers/PropertiesParserTests.swift(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build-and-test (macos)
- GitHub Check: build-and-test (visionos)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (ios)
- GitHub Check: build-and-test (maccatalyst)
🔇 Additional comments (9)
Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax.swift (4)
24-25: Well-structured refactoring of extension blocksSplitting the extensions by related functionality improves code organization and readability.
28-30: Access control level implementation nicely refactoredThe implementation now delegates to a private helper method, enabling code reuse while maintaining behavior.
32-34: Good implementation of setter access control detectionThe new property correctly looks for setter-specific access control with appropriate fallback to the general access control level.
36-41: Well-designed helper method for access control detectionThe implementation efficiently filters modifiers by detail and finds the first matching access control level. The lazy evaluation is a nice performance optimization.
Tests/PrincipleMacrosTests/Parsers/PropertiesParserTests.swift (5)
26-26: Proper assertion for setter access level on immutable propertyFor immutable properties, the setter access level should match the general access level.
37-38: Good test case for setter-specific access controlChanged from simple private variable to testing a setter-specific access control modifier.
63-63: Appropriate assertion for property without explicit setter access controlThe test correctly verifies that
setterAccessControlLevelis nil when no setter-specific access control is specified.
83-83: Correct assertion for computed property's setter access levelFor computed properties without explicit setter access control, the setter access level should match the general access level.
103-103: Correct assertion for property without any access control specifiedTest correctly verifies that both the property and setter access levels are nil when no access control is specified.
| } | ||
|
|
||
| extension WithModifiersSyntax { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding test for combined access control modifiers
The current implementation handles setter-specific modifiers well, but I don't see any test for combined modifiers like public private(set) var.
🏁 Script executed:
#!/bin/bash
# Check if there are test cases for combined access control modifiers like 'public private(set) var'
grep -r "public\s\+private(set)" --include="*.swift" Tests/Length of output: 61
To ensure we haven’t missed any existing tests around public private(set), let’s broaden our search:
🏁 Script executed:
#!/bin/bash
# Search for the exact combined modifier and any private(set) usage in Swift tests
grep -R -E "public\s+private\(set\)" --include="*.swift" Tests/ -n
grep -R "private(set)" --include="*.swift" Tests/ -nLength of output: 238
Add tests for combined access control modifiers
We already have coverage for private(set) in Tests/PrincipleMacrosTests/Parsers/PropertiesParserTests.swift (line 37), but there’s no test exercising a declaration like
public private(set) var myVar = "Hello, world!"Please add at least one parser test for the combined modifier scenario. For example, in PropertiesParserTests.swift:
func testPublicPrivateSetVar() throws {
let source = "public private(set) var myVar = 42"
let syntax = try parseDeclaration(source)
XCTAssertTrue(syntax.modifiers.contains(.public))
XCTAssertTrue(syntax.modifiers.contains(.privateSet))
// …additional asserts…
}🤖 Prompt for AI Agents
In Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax.swift around
lines 42 to 44, the review suggests adding a test for combined access control
modifiers like 'public private(set) var'. To fix this, add a new parser test in
Tests/PrincipleMacrosTests/Parsers/PropertiesParserTests.swift that parses a
declaration with combined modifiers, such as "public private(set) var myVar =
42", and assert that both .public and .privateSet modifiers are correctly
recognized in the syntax tree.
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
Summary by CodeRabbit
New Features
Bug Fixes
Tests