-
Notifications
You must be signed in to change notification settings - Fork 267
fix: correctly handle prefixed enums and static instance fields for prefer-moving-to-variable #1123
Conversation
Kudos, SonarCloud Quality Gate passed! |
Dart Code Metrics unused files report of dart_code_metrics. ✅Summary
|
Dart Code Metrics analyze report of dart_code_metrics. ✅Summary
|
Codecov Report
@@ Coverage Diff @@
## master #1123 +/- ##
=======================================
Coverage 85.54% 85.54%
=======================================
Files 349 349
Lines 7870 7873 +3
=======================================
+ Hits 6732 6735 +3
Misses 1138 1138
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
AnotherEnum.anotherValue; | ||
AnotherEnum.firstValue; | ||
|
||
prefix.SomeValue.firstValue; |
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.
try referencing prefix.SomeValue.firstValue
repeatedly
I believe that would match the case where I was seeing false positives.
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.
Yes, that's it! Thank you!
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.
Hm, actually, if I duplicate the line 5 AnotherEnum.anotherValue;
the rule will also trigger. So it's not connected to prefixes, maybe you remember other cases?
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.
The strange thing is removing the prefixes did seem to fix the issue in DevTools. That said, I wouldn't want the rule to trigger for duplicated AnotherEnum.anotherValue
case either. Specifying the enum value name twice seems more idiomatic than defining an unneeded local variable.
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.
Agree. And maybe the same should work for the class level static variables. You probably wouldn't want to assign this type of calls to a variable.
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.
Agree that I wouldn't want this firing for class to class level static variables either.
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.
Sorry for misleading, it was a problem exactly with the prefixes 🤦
Now fixed, thank you!
…refer-moving-to-variable
…oving-to-variable
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.
What is the purpose of this pull request? (put an "X" next to an item)