-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add ProhibitedSuperRule #971
Conversation
Current coverage is 81.69% (diff: 49.15%)@@ master #971 diff @@
==========================================
Files 127 129 +2
Lines 6263 6322 +59
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 5136 5165 +29
- Misses 1127 1157 +30
Partials 0 0
|
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.
Great rule!
let name = dictionary["key.name"] as? String | ||
else { return [] } | ||
|
||
let substructure = (dictionary["key.substructure"] as? [SourceKitRepresentable]) ?? [] |
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.
could move this variable declaration down, and merge both guard
groups together
|
||
import SourceKittenFramework | ||
|
||
public struct ProhibitedSuperRule: ConfigurationProviderRule, ASTRule, OptInRule { |
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.
why should this be an opt-in rule? seems like it should be enabled by default...
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 had considered it, but had decided to make it opt-in as the rule is based on method signature and some of the signatures are not particularly unique. Without adding additional checks to see if the class is a descendent of the correct object, I was worried there would likely be some false positives. I thought that might be a good addition in a future PR.
But if you feel that those developers should either change the names of their methods or mark it as excluded in the configuration, I'm happy to make it opt-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.
Yeah actually, I wrote this before realizing that this rule didn't check for superclass inheritance to make sure that only the correct methods were being flagged, so opt-in is probably best. 👌
configuration.resolvedMethodNames.contains(name) && | ||
dictionary.enclosedSwiftAttributes.contains("source.decl.attribute.override") | ||
else { return [] } | ||
|
||
let substructure = (dictionary["key.substructure"] as? [SourceKitRepresentable]) ?? [] |
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 could also be folded into the guard
statement above since it falls back on []
if the conditional cast fails, which causes to return []
anyway
Resolves #970:
Rule: Methods calling super that should not
.Summary
Adds an opt-in
ProhibitedSuperRule
that catches:NSFileProviderExtension.providePlaceholder(at:completionHandler:)
NSTextInput.doCommand(by:)
NSView.updateLayer()
UIViewController.loadView()
Rule is based on
OverriddenSuperCallRule
.