skip tuples for no_magic_numbers rule
#5327
Conversation
Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoMagicNumbersRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoMagicNumbersRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoMagicNumbersRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoMagicNumbersRule.swift
Outdated
Show resolved
Hide resolved
mildm8nnered
force-pushed
the
mildm8nnered-ignore-magic-numbers-in-tuples-again
branch
from
cb70268 to
101d4da
Compare
|
Looking at the OSS findings, there's another case I haven't thought of before. If there are more complicated expression in a tuple initialization like in |
So I thought the OSS violations we ended up with looked ok, but Maybe drop back to just allowing
That would still allow
or we could go further and require that the tuple elements all be simple expressions. |
I think that's the way to go. The rule currently triggers on |
Sounds good to me - |
Just to be sure: Also |
mildm8nnered
force-pushed
the
mildm8nnered-ignore-magic-numbers-in-tuples-again
branch
from
a12ad34 to
9f2d668
Compare
ok - so I think I have this working. As long as every element of the RHS tuple is an Integer or Float, we will allow it through, so
|
| Example("let a = b / 100.0") | ||
| Example("let a = b / 100.0"), | ||
| Example("let (lowerBound, upperBound) = (400, 599)"), | ||
| Example("let a = (5, 10)") |
There was a problem hiding this comment.
Let's also have a test for labeled tuple elements and a tuple used somewhere else that triggers the rule.
After that, I promise to accept the change. 😇
There was a problem hiding this comment.
So I added those examples, and also extended the allowed tuple elements to include string literals, as cases like let notFound = (404, "Not Found") seemed reasonable to skip as well.
There was a problem hiding this comment.
But hold off on merging for right now @SimplyDanny - I want to checkout the OSS results, which now seem to have suspiciously lower fixes compared to before
There was a problem hiding this comment.
ok - that looks better. Should be good to go now 🤞 once CI passes
There was a problem hiding this comment.
The strings and booleans make me think again. We now have:
let a = (3, 4) // ok
let a = (3, "a") // ok
let a = (3, b) // triggers on 3
let a = (3, (4, 5)) // triggers on all numbers
let a = (true, 4) // ok
let a = (3, 4 + 5) // triggers on all numbersDoes this make sense? Is it consistent and logical? 🤔 We might need to come up with a better set of rules for tuples I'm afraid.
There was a problem hiding this comment.
So I totally get your point, but actually, all of the above examples would have been violations before, and there were enough reasonably allowable fixed violations in the OSS tests, that I think the change will add some value for some users.
let a = (3, b) // triggers on 3
let a = (3, (4, 5)) // triggers on all numbers
let a = (3, 4 + 5) // triggers on all numbers
will still be violations, but in the OSS code, there were few or none of these.
Users of the above examples would not be put at any disadvantage by this change - they would just need to disable the rule, like they would today.
I could definitely see a case for allowing let a = (3, (4, 5)), but if someone wants that to pass, they can always request that we extend the coverage.
There was a problem hiding this comment.
Fair enough. Let's have it merged then. Good job!
mildm8nnered
force-pushed
the
mildm8nnered-ignore-magic-numbers-in-tuples-again
branch
from
e08d78f to
3d21880
Compare
mildm8nnered
deleted the
mildm8nnered-ignore-magic-numbers-in-tuples-again
branch
Now ignores magic numbers in tuple assignments like
let (httpStatusCodeErrorLowerBound, httpStatusCodeErrorUpperBound) = (400, 599)OSS checks look good to me.
Resolves #5305 by skipping magic numbers in tuple expressions altogether.