-
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
Add if_only
option to rule conditional_returns_on_newline
#2308
Add if_only
option to rule conditional_returns_on_newline
#2308
Conversation
5332a56
to
51a9a62
Compare
Generated by 🚫 Danger |
Codecov Report
@@ Coverage Diff @@
## master #2308 +/- ##
==========================================
- Coverage 92.16% 92.15% -0.02%
==========================================
Files 292 294 +2
Lines 14738 14780 +42
==========================================
+ Hits 13584 13620 +36
- Misses 1154 1160 +6
Continue to review full report at Codecov.
|
@@ -30,19 +30,21 @@ public struct ConditionalReturnsOnNewlineRule: ConfigurationProviderRule, Rule, | |||
) | |||
|
|||
public func validate(file: File) -> [StyleViolation] { | |||
let pattern = "(guard|if)[^\n]*return" | |||
let pattern: String = configuration.ifOnly ? "(if)[^\n]*return" : "(guard|if)[^\n]*return" |
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 usually don't add explicit types
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.
Removed.
return file.rangesAndTokens(matching: pattern).filter { _, tokens in | ||
guard let firstToken = tokens.first, let lastToken = tokens.last, | ||
SyntaxKind(rawValue: firstToken.type) == .keyword && | ||
SyntaxKind(rawValue: lastToken.type) == .keyword else { | ||
return false | ||
} | ||
|
||
return ["if", "guard"].contains(content(for: firstToken, file: file)) && | ||
let searchTokens: [String] = configuration.ifOnly ? ["if"] : ["if", "guard"] |
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 usually don't add explicit types
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 was merely to speed up the compiler since it seems to be slow on type-checking ternary operators. But I can remove it, if you want.
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.
Removed.
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.
Can you please add some tests to make sure we don't break this in the future?
I recommend removing the AutomaticTestableRule
conformance and creating a new ConditionalReturnsOnNewlineRuleTests
that has a test for the default configuration and one for setting if_only: true
@marcelofabri I just removed the |
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.
Looks good! Thanks! 🚀
Fixes #2307.