-
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
Fix ExplicitTypeInterfaceRule in groups and statements #2588
Fix ExplicitTypeInterfaceRule in groups and statements #2588
Conversation
This looks good to me, thanks for re-filing the PR, it was easier to review and understand. Let's merge once CI is happy unless @marcelofabri or other maintainers have feedback. |
} | ||
|
||
var caseExpressionRanges: [NSRange] { | ||
return ranges(with: "source.lang.swift.expr.tuple", for: "source.lang.swift.structure.elem.expr") |
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.
nitpick: the tuple kind should be defined in SwiftExpressionKind
@@ -64,4 +64,153 @@ class ExplicitTypeInterfaceRuleTests: XCTestCase { | |||
|
|||
verifyRule(description, ruleConfiguration: ["allow_redundancy": true]) | |||
} | |||
|
|||
func testEmbededInStatements() { | |||
let nonTriggeringExamples = [ |
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.
it there a reason to not define all the examples in the rule description?
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 considered them to be edge cases, and hid them from the rule's description.
Also, the testSwitchCaseDeclarations
tests are only running above Swift 4.1. If I would move them to the default place, I must increase the base version of the rule to 4.1.
Shall I move it to the rule's description though?
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.
Sounds good to me 👍
Thanks for fixing this, @dirtydanee! Just had two minor comments. |
1b881ae
to
4634467
Compare
Huh, I wonder if the regressions reported by oss-check are real. @dirtydanee can you run it locally to double check? |
@marcelofabri On the I get errors locally constantly, when running |
@dirtydanee I was talking about the times reported by oss-check (the build itself seem fine). You can run it by using |
@marcelofabri Sorry for the confusion! Here are my results for 10 iterations:
|
Those local perf numbers look good. |
Thanks @dirtydanee! |
First of all, thanks for all the efforts on the project! In this PR I am attempting to fix #2154
Main changes to the original rule's implementation:
Added checks if a given declaration kind:
for
orforEach
statementMoved functions to an extension on Dictionary
Added test cases covering the new implementation to
ExplicitTypeInterfaceRuleTests.swift