-
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 new prefer_key_path
rule
#5548
Conversation
@mildm8nnered: I'd like to hear your opinion. The rule doesn't seem to be so involved. Any more cases or shortcomings you can think of? |
247038c
to
ff7a7e3
Compare
ff7a7e3
to
71a5b62
Compare
Just trying it on my local codebase, which I expect to have a lot of violations, pretty much like the OSS findings. |
So most hits look good, but I think this might be a false alarm (where there is no property named
|
The rule would complain about the closure and suggest |
71a5b62
to
d75d058
Compare
d75d058
to
1b4563e
Compare
8dffce7
to
2927aa4
Compare
I'm still unsure about this rule. As we see in the list of OSS findings, there will potentially be a lot of violations in existing code bases (78 in SwiftLint), most of them on Fixing only a few (but most of the) findings seems to be against the philosophy of SwiftLint. Are there other rules that fix only partially? Would this be acceptable? Or do people expect a |
fb1dae2
to
30c1cd7
Compare
So I restricted the rule to known standard functions by default and there is an option to enable it for all other closures as well. I actually doubt that anyone would enable it to be honest, especially when it comes to auto-fixes. @mildm8nnered: May I ask you again to run this rule with |
30c1cd7
to
176b8f4
Compare
Luckily, there is an accepted proposal that is going to make the auto-fixed code compile in this special case. |
617298f
to
eed7193
Compare
Could you check again with the |
So |
Thank you! That's good news. So the PR can finally land. Would you like to give a vote, @mildm8nnered? |
eed7193
to
3f3ff42
Compare
This new rule triggers on code like
which can be replaced by
as of Swift 5.2.
Automatic fixes fail once the argument label for the closure is named. The fix would need to know its name and insert it in the function call. But that's impossible for a syntax-based rule. We could do auto-fixes for simple cases like
map
andfilter
(which represent most of the findings in OSS projects and SwiftLint itself) at least. I'm not sure if partial fixes are a good thing though.