-
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
Adding SyntacticSugar rule #851
Conversation
@@ -77,7 +77,8 @@ extension File { | |||
} | |||
set { | |||
if newValue { | |||
responseCache.values[cacheKey] = Optional<[String: SourceKitRepresentable]>.None | |||
let value: [String: SourceKitRepresentable]? = nil | |||
responseCache.values[cacheKey] = value |
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've tried just using responseCache.values[cacheKey] = nil
or responseCache.values[cacheKey] = .None
but a test started to fail 😬
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, we are using Optional.None
as failed state of response from SourceKit.
Current coverage is 85.75% (diff: 100%)
|
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.
Thanks for doing this! 🙏
Looks good to me other than I commented. 👍
should this be opt-in?
IMO, I can accept this rule as opt-out.
public func validateFile(file: File) -> [StyleViolation] { | ||
let types = ["Optional", "ImplicitlyUnwrappedOptional", "Array", "Dictionary"] | ||
|
||
let pattern = "\\b(" + types.joinWithSeparator("|") + ")\\b\\s*<.*?>" |
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.
Second \\b
would not be needed.
I've updated the regex in ab7ede0 🚀 Thanks for reviewing this 💯 |
Could you please resolve conflicts? |
Done! |
@realm/swiftlint: How do you think of that this rule should be opt-in or opt-out? |
This looks great, and I think it's fine to keep on-by-default. I'll address the merge conflict and make an new PR. |
Merged in #896. |
I've implemented the "Syntactic Sugar" from https://github.com/raywenderlich/swift-style-guide#syntactic-sugar (also mentioned in #319).
It should be easy to make this rule correctable, but I think it's better to do that on a later PR.
Also, should this be opt-in? (It's not currently)