-
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
No Grouping Extension Rule #1789
Conversation
I can see a good configuration for this might be |
I think this can be a future improvement, since (in my experience) it's not common to have extensions on nested types that you own. |
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 really liked the implementation 💯
Just some minor comments. Also, I'm not a big fan of this rule name, since "ban" is too negative and strong IMO. Can we think about something else? Maybe NoGroupingExtensionRule
?
|
||
extension GroupingExtensionBanRule { | ||
|
||
struct Element { |
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.
what do you think about making this a private top level declaration? This declaration shouldn't be visible for other classes anyway.
} | ||
|
||
private func findAllElements(in dictionary: [String: SourceKitRepresentable], | ||
ofTypes types: [SwiftDeclarationKind], |
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.
maybe change ofTypes
to of
?
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.
Also, what do you think about changing types
to Set<SwiftDeclarationKind>
?
namespace: [String] = []) -> [Element] { | ||
|
||
return dictionary.substructure | ||
.flatMap { subDict -> [Element] in |
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.
do you need to break line here (before .flatMap
)
Fair enough. My personal opinion is that Swiftlint needs a common suffix for discouraged (banned) practices, in order to easily navigate the rules and add consistency to naming. Starting rules with a common word like "No" and "Discourage" might group those rules and hide the actual name "GroupingExtension". I will proceed with your suggestion now, but thought I shed light on the issue above for possible future enhancements. |
Weekend is finally here! All requested changes are done. Btw, what do you think about the |
Codecov Report
@@ Coverage Diff @@
## master #1789 +/- ##
==========================================
+ Coverage 88.17% 88.18% +<.01%
==========================================
Files 223 224 +1
Lines 11027 11068 +41
==========================================
+ Hits 9723 9760 +37
- Misses 1304 1308 +4
Continue to review full report at Codecov.
|
@Mazyod could you just rebase to fix the conflict? Thanks! |
TIL how to resolve conflicts right from within github 😅 .. I pulled and tested, now getting file length lint error. I think I should resolve that? |
Adding |
Yeah, we can do that by now 🤷♂️ |
Also, can you try rebasing to get rid of 4c7f262? 😬 |
Ah, I thought you guys just squash and merge. @marcelofabri is that an option? |
I can do that 👍 |
@@ -133,6 +135,10 @@ class RulesTests: XCTestCase { | |||
verifyRule(FunctionParameterCountRule.description) | |||
} | |||
|
|||
func testNoGroupingExtension() { |
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.
just one more thing: we try to keep this list sorted. Can you please move it (and run make sourcery
again)? 😅
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.
Of course! Sorry about that.
Hello again!
This attempts to implement the opt-in rule for warning against grouping extensions (couldn't think of a better name).
Element
which we can use.enum
,struct
, &class
types against the extension names.Fixes #1767