-
Notifications
You must be signed in to change notification settings - Fork 630
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 —nilinit
for redundantNilInit
rule
#1680
Add —nilinit
for redundantNilInit
rule
#1680
Conversation
The nil default value may be missing here
var foo: String? | ||
var foo: String? = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did I miss adding nil as a default value here? If I understood it wrong, I will change it back, the same below.
var foo: String? = nil | ||
Text(foo ?? "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did I miss adding nil as a default value here? If I understand it wrong, I will change it back, same as above.
…ilInit # Conflicts: # Sources/Options.swift
I fixed some issues and can now start the review. |
Thank you - this looks excellent. One thing I didn't previously consider is the case where the value is already being initialized in the init method, e.g. class Foo {
var foo: Int?// probably don't want to add = nil here?
init() {
foo = nil
}
} I checked and I don't think setting the value to nil twice will actually cause any compilation problems or changes in behavior, so it's probably fine to consider handling this an enhancement rather than a blocker for the feature though. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1680 +/- ##
===========================================
+ Coverage 92.81% 95.18% +2.36%
===========================================
Files 20 20
Lines 21162 22964 +1802
===========================================
+ Hits 19642 21858 +2216
+ Misses 1520 1106 -414 ☔ View full report in Codecov by Sentry. |
If there is a rule in the future (I'm not sure if there is one now): "All properties that do not rely on external input should have their initial values set when they are defined, rather than set in the init method" (Let’s call it In my opinion, the correct solution to the situation you proposed is as follows: // --nilinit insert
class Foo {
- var foo: Int?
+ var foo: Int? = nil
// If init is empty, the entire init should also be removed.
// But I remember that there seems to be another rule that can automatically remove empty methods.
// I'm not sure how SwiftFormat handles the same parts between the two rules, is it implemented twice?
//
// Considering that the `remove` option does not handle the situation of `var foo: Optional<Int> = nil`
// (there are other rules to process `Optional<Int>` as `Int?`),
// so I guess there should be no need to delete the empty init method here?
init() {
- foo = nil
}
} I'm not currently dealing with the init internals and I'd like to hear your thoughts. If After I implement the above logic, |
Please let me list other situations: // --nilinit insert
class Foo {
- var foo: Int?
+ var foo: Int? = nil
init() {
- foo = nil
- var foo: String?
+ var foo: String? = nil
}
} // --nilinit insert-only-properties
class Foo {
- var foo: Int?
+ var foo: Int? = nil
init() {
- foo = nil
var foo: String? // nil will not be automatically inserted
}
} // --nilinit insert-only-variables
class Foo {
var foo: Int? // nil will not be automatically inserted
init() {
foo = nil
- var foo: String?
+ var foo: String? = nil
}
} |
@rakuyoMo thanks for the update - I was not actually expecting you to fix this!
This is absolutely fine not to include. |
Related #1677 .
Added
—nilinit
option toredundantNilInit
rule, its value includesinsert
andremove
. Among them,remove
is consistent with the previous logic, andinsert
is a new logic. It will addnil
as the default value after the optional value type according to the opposite rules ofremove
.I added documentation and test cases and all of them passed.
This is my first PR for this project. If I did something wrong or if there is anything else that needs to be modified, please let me know.