SimplyDanny
commented
2 months ago @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?
Open SimplyDanny opened 2 months ago
SimplyDanny
commented
2 months ago @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?
SwiftLintBot
commented
2 months ago | 17 Messages | |
|---|---|
| :book: | Linting Aerial with this PR took 0.89s vs 0.89s on main (0% slower) |
| :book: | Linting Alamofire with this PR took 1.26s vs 1.23s on main (2% slower) |
| :book: | Linting Brave with this PR took 7.33s vs 7.29s on main (0% slower) |
| :book: | Linting DuckDuckGo with this PR took 4.08s vs 4.12s on main (0% faster) |
| :book: | Linting Firefox with this PR took 10.45s vs 10.36s on main (0% slower) |
| :book: | Linting Kickstarter with this PR took 9.14s vs 9.09s on main (0% slower) |
| :book: | Linting Moya with this PR took 0.52s vs 0.51s on main (1% slower) |
| :book: | Linting NetNewsWire with this PR took 2.5s vs 2.54s on main (1% faster) |
| :book: | Linting Nimble with this PR took 0.74s vs 0.77s on main (3% faster) |
| :book: | Linting PocketCasts with this PR took 8.07s vs 7.96s on main (1% slower) |
| :book: | Linting Quick with this PR took 0.42s vs 0.42s on main (0% slower) |
| :book: | Linting Realm with this PR took 4.62s vs 4.65s on main (0% faster) |
| :book: | Linting Sourcery with this PR took 2.36s vs 2.36s on main (0% slower) |
| :book: | Linting Swift with this PR took 4.45s vs 4.43s on main (0% slower) |
| :book: | Linting VLC with this PR took 1.21s vs 1.22s on main (0% faster) |
| :book: | Linting Wire with this PR took 17.31s vs 17.25s on main (0% slower) |
| :book: | Linting WordPress with this PR took 12.87s vs 12.84s on main (0% slower) |
Generated by :no_entry_sign: Danger
mildm8nnered
commented
2 months ago @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?
Just trying it on my local codebase, which I expect to have a lot of violations, pretty much like the OSS findings.
mildm8nnered
commented
2 months ago @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?
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 otherResults). I adapted this from proprietary code that I saw the hit with - I have not tested this specific example code here, but if it doesn't get a hit I can dig in deeper on my side.
guard let otherResults = results as? [SomeClass] else {
return
}
let dictionary = Dictionary(grouping: ↓otherResults) {
$0.someProperty
}@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?
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
otherResults). I adapted this from proprietary code that I saw the hit with - I have not tested this specific example code here, but if it doesn't get a hit I can dig in deeper on my side.guard let otherResults = results as? [SomeClass] else { return } let dictionary = Dictionary(grouping: ↓otherResults) { $0.someProperty }
The rule would complain about the closure and suggest Dictionary(grouping: otherResult, by: \.someProperty). Looks okay to me.
SimplyDanny
commented
1 day ago 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 .map, .filter, ... These are cases that could be and should be (due to their sheer amount) auto-fixed. However, fixing all findings will very likely lead to wrong code due to missing parameter names for custom functions accepting closures.
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 swiftlint lint --fix run to repair all violations?
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
mapandfilter(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.