-
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
Prefer Swift constructors to legacy ones #242
Conversation
] | ||
) | ||
|
||
public func validateFile(file: File) -> [StyleViolation] { |
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 have a few suggestions:
- It's more performant to only scan the file for matches once, which can be done by merging all the regular expressions into one.
- Instead of excluding comments and strings, we should check if the match is a
SyntaxKind.Identifier
which is more semantically correct, even though it probably won't make a difference in practice.
So this function could be refactored to this:
public func validateFile(file: File) -> [StyleViolation] {
let constructors = ["CGRectMake", "CGPointMake", "CGSizeMake", "CGVectorMake",
"NSMakeRange"]
let pattern = "\\b(" + constructors.joinWithSeparator("|") + ")\\b"
return file.matchPattern(pattern, withSyntaxKinds: [.Identifier]).map {
StyleViolation(ruleDescription: self.dynamicType.description,
location: Location(file: file, offset: $0.location))
}
}
This is great @marcelofabri! I only had a few minor suggestions regarding the implementation. Could you also add an entry for this to the changelog? Thanks for doing this. |
// | ||
|
||
import SourceKittenFramework | ||
import SwiftXPC |
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.
We don't use SwiftXPC in this file, so this import statement can be removed.
🎉 |
👍 awesome work! |
Prefer Swift constructors to legacy ones
What do you think, @jpsim?