-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove extensions from LowerACLThanBodyRule #2164
Conversation
case .fileprivate: return 1 | ||
case .internal: return 2 | ||
case .public: return 3 | ||
case .open: return 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change might be a little too weird, in which case I can make this private to LowerACLThanBodyRule. For that use case private == fileprivate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a public interface, I'm a bit reluctant in changing its meaning. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this Comparable conformance in my last PR, so I'm assuming this rule is still the only consumer, but I'm definitely happy to move it to be private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this into this rule's file
Because of some oddities in the Swift extension ACL ruless, linting ACL inside extensions isn't trivial because of a few cases: 1. Extensions cannot be open, but members inside them can: ``` open extension Foo { // Not valid open func bar() {} } ``` 2. Extensions cannot have an ACL modifier if they conform to a protocol: ``` public extension Foo: Bar {} // Not valid ``` 3. Extensions that do have an ACL modifier affects the inner body, making changes to this slightly riskier: ``` public extension Foo { func bar() {} // implicitly public } ``` With this change we just sidestep all these problems, and instead opt to only lint non extension types. I think this tradeoff is worth it so that this can be enabled and catch issues in classes/structs/enums
Codecov Report
@@ Coverage Diff @@
## master #2164 +/- ##
=========================================
Coverage ? 91.75%
=========================================
Files ? 268
Lines ? 13176
Branches ? 0
=========================================
Hits ? 12089
Misses ? 1087
Partials ? 0
Continue to review full report at Codecov.
|
since this was publicly added and is generally useful to consumers of SwiftLintFramework.
Thanks @keith. I re-added |
Sounds good! |
Because of some oddities in the Swift extension ACL rules, linting ACL
inside extensions isn't trivial because of a few cases:
making changes to this slightly riskier:
With this change we just sidestep all these problems, and instead opt to
only lint non extension types. I think this tradeoff is worth it so that
this can be enabled and catch issues in classes/structs/enums