Add consider_default_literal_types_redundant option to RedundantTypeAnnotationRule #4756
Conversation
garricn
force-pushed
the
add-redundant-type-annotation-configuration
branch
from
70ebae1 to
2cd28bd
Compare
|
An observation for this rule in general: The exception for I wonder if we should rather extend the rule for these cases and rename the new option @revolter: You added the exception for |
garricn
force-pushed
the
add-redundant-type-annotation-configuration
branch
3 times, most recently
from
e16a7f6 to
1302af9
Compare
Hi @SimplyDanny, thank you for your observation. The examples you gave are not currently triggering examples and booleans are the only literals that currently violate this rule. It makes sense that numeric and string literals are not violations since you may need to be explicit that the type in |
|
Well, without the explizit types, the variables The current implementation checks exactly for the combinations Moreover, a variable being assigned a Boolean literal is not necessarily a The exception for |
garricn
force-pushed
the
add-redundant-type-annotation-configuration
branch
from
1302af9 to
5c2907c
Compare
Thank you for your reply @SimplyDanny. The original intent for the special handling of Bool is not discernible from the PR or the issue. My guess is that we would not want any of the following to be violations because, in certain cases, we might need to be explicit about the type: let someInt: Int = 0
let someDouble: Double = 0
let someFloat: Float = 0
let someString: String = "foo"
let someStaticString: StaticString = "bar"
let someCustomBool: CustomBool = falseHowever, in the case of Bool, we do not need to be explicit when defining one because https://developer.apple.com/documentation/swift/expressiblebystringliteral#conforming-types Granted, you can create your own type that is Although I think your point is a valuable one, I believe it is outside the scope of this PR and so I'd like to suggest the following way forward:
|
That might be an acceptable way to go. However, I still take exception to the option's name |
garricn
force-pushed
the
add-redundant-type-annotation-configuration
branch
from
5c2907c to
9e25db1
Compare
garricn
force-pushed
the
add-redundant-type-annotation-configuration
branch
from
9e25db1 to
99ddb9f
Compare
|
Reading through the comments, it really feels like the decision to have I would like to reiterate that I would like to kindly suggest an alternative way forward:
With this approach, there is no need for a configuration option at all. @SimplyDanny Would this alternative approach be accepted? And do you prefer Thank you 🙏 |
|
For clarity: I was the one that requested this (in #3423), but I didn't make any code change for it. As far as the current situation goes, I would be fine with reverting that change completely.
but this can change in the future, so I would say that considering let test: Bool = trueto be redundant is a guess, while considering let test: URL = URL()to be redundant is a certainty. |
|
I'd like to avoid exceptions for certain cases that actually should follow the same rules as others. Breaking changes are acceptable if they really fix an inconstancy or harmonize implementations. The exception for booleans bugs me, with or without the Types can conform to any Since there might be people not agreeing on the statement that default types can be considered redundant, my favorite is still this let k: Int = 0
let d: Double = 0.0
let s: String = ""
let b: Bool = falsewould all trigger while let k: Int8 = 0
let d: Double = 0
let s: Name = ""
let b: OnOff = truewould not. |
|
@SimplyDanny Thank you for weighing in. Very much appreciated 👍 So, to confirm, you are suggesting that on this PR, the As far as treating |
Yes, that's what feels most reasonable to me. However, the default should be Any noticeable flaw in my reasoning? |
|
@SimplyDanny Sounds good 👍 … And may treating |
Well, ideally we would have all types properly supported right from the new option's introduction on. 😉 |
garricn
force-pushed
the
add-redundant-type-annotation-configuration
branch
from
99ddb9f to
5ae7962
Compare
garricn
changed the title
consider_default_literal_types_redundant option to RedundantTypeAnnotationRule
garricn
changed the title
consider_default_literal_types_redundant option to RedundantTypeAnnotationRuleconsider_default_literal_types_redundant option to RedundantTypeAnnotationRule
garricn
changed the title
consider_default_literal_types_redundant option to RedundantTypeAnnotationRule
|
@SimplyDanny Thanks again for all the feedback. I investigated adding validations for Int, Double and String literals and determined that an overhaul of the rule, potentially adopting SwiftSyntax, would likely be required. Since that would significantly increase the scope and complexity of the PR, I propose that the PR is limited to adding the new config option only. I've updated to use the option renamed as you requested, and it's working really well! My hope is that you will consider merging this as is and allow the additional type support to be added later. I look forward to hearing back from you. Thank you 😄 |
The rewrite with SwiftSyntax is now done with #5389 just being merged. I would still appreciate you implementing all cases to have this in a complete and reasonable shape. With the rewrite, this ought not to be that complex. Otherwise, we need to wait for me or another contributor to find the time to address this. |
garricn
force-pushed
the
add-redundant-type-annotation-configuration
branch
from
de97b68 to
a53efa1
Compare
|
@SimplyDanny The rewrite of the rule with SwiftSyntax makes this much more straightforward! Very cool! 👍 @garricn Thank you for implementing this! ❤️ |
Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantTypeAnnotationRule.swift
Outdated
Show resolved
Hide resolved
garricn
force-pushed
the
add-redundant-type-annotation-configuration
branch
from
a53efa1 to
3b222cf
Compare
There was a problem hiding this comment.
Thanks again Garric! Looking forward to using this new option! 👍
I added two small suggestions, but they are very minor, so only use if you see a benefit.
I do have one question though, which is whether we need to indicate somewhere that Bool literals will no longer be considered redundant by default. Since this is a change in behavior, does it need to be listed as a "breaking" change?
Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantTypeAnnotationRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantTypeAnnotationRule.swift
Outdated
Show resolved
Hide resolved
Yes, I'd like that. |
Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantTypeAnnotationRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantTypeAnnotationRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantTypeAnnotationRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantTypeAnnotationRule.swift
Outdated
Show resolved
Hide resolved
garricn
force-pushed
the
add-redundant-type-annotation-configuration
branch
from
19e20cd to
c044d71
Compare
|
@SimplyDanny Thanks for all your guidance and attention on this! Very much appreciated ⭐ @garricn Great work! Thank you again for making this improvement. |
|
@SimplyDanny That's great! 👏 |
consider_default_literal_types_redundantoption toRedundantTypeAnnotationConfigurationvisitPostmethodsisRedundantmethod to include and useconsiderLiteralsRedundantforBool,Double,IntandStringBy default, literals for
Bool,Double,IntandStringwill be considered not redundant. For example:var isEnabled: Bool = true. This is an intentional change in behavior. See the comments below for additional details.Important
The
consider_default_literal_types_redundantoption must be set totrueto maintain the previous behavior.