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
Don't trigger explicit_type_interface
on if case statements
#2853
Conversation
Although the fix worked for the sample inserted in the I am looking into it to check if I am building correctly and will push another commit as soon as I have an update :) |
Thanks for the contribution! I'd look into fixing the existing logic ( |
Thanks, I will look into it! I had some trouble using the AST because in these scenarios we do not have a I do believe the "if case" statement is the only place this will happen, but since I know very little of the AST, I thought it was safer to use regex. I would love to get an opinion about this, do you think it is safe to use this pattern in the AST ( |
I found another scenario that my first commit did not caught, if the if case has more than one property, for example: enum Foo {
case caseOne(propOne: Int, propTwo: Int)
}
let testA: Foo = .caseOne(propOne: 2, propTwo: 4)
if case let Foo.caseOne(propOne, _) = testA { } The AST returns a Running Message: Linting Aerial with this PR took 3.98s vs 4.08s on master (2% faster) And handling the output of it with
I could not run only the script at the moment, so the performance data may not be that accurate, if necessary I can run it again later and post the full output here :) |
Wow, didn't know the comment from Danger would auto update :) But just noticed that this also fixes warnings triggered in capture lists, where apparently it is not possible to explicitly define the type (this is the case of line 113 of NotificationViewController.swift in WordPress, for example) |
let's get this in 🙏 |
Can you please rebase it against master and add some tests for the capture list in closures? Thanks! |
Yes, sure, I'll rebase and add the tests! :) |
4e09cad
to
4fe802e
Compare
Sorry it took so long! I just rebased the branch and when I was to implement the tests for the Capture groups, I saw there is already a I still implemented locally some tests, following the same patterns of some fixed violations according to the Bot (capture groups with one word or with equal symbol), but those did not pass. I think it would make more sense to work on this on a separate issue, what do you think @marcelofabri ? I could start working on this tomorrow morning :) One more thing, I also included a test to catch definitions, such as this one here, that is one of the corrections of this PR. But I had to disable type_body_length on ExplicitTypeInterfaceRuleTests, is this ok? |
This issue has been automatically marked as stale because it has not had any recent activity. Please comment to prevent this issue from being closed. Thank you for your contributions! |
The
explicit_type_interface
is being triggered onif case let
statements with enum with types.For exemple, this code will trigger a warning:
while this one will not:
On
if case Enum.case(variable) = enum
the parent of the variable structure is ofcall
type, because of this the current validation forswitch
andcases
are not workingI was in doubt if a better solution would be:
if case let
if case
I think the first one seems to follow better the pattern I saw on the code, but I am not comfortable assuming every combination of "Parent: Call, child: Var" is indeed a
if case let
situation. With this one, checking if the grandparent of the var is an if statement should also be good, what makes it even more complicated.The second one seems a little bit inefficient, but is clear when reading and seems more assertive to me.