149-new-lint-avoid_final_with_getter #161
Conversation
There was a problem hiding this comment.
Great work so far, @4akloon! I've added a couple of suggestions here - please, take a look.
lib/src/lints/avoid_final_with_getter/avoid_final_with_getter_visitor.dart
Outdated
Show resolved
Hide resolved
lib/src/lints/avoid_final_with_getter/avoid_final_with_getter_visitor.dart
Outdated
Show resolved
Hide resolved
lib/src/lints/avoid_final_with_getter/avoid_final_with_getter_visitor.dart
Outdated
Show resolved
Hide resolved
| final int _myField = 0; | ||
|
|
||
| int get myFieldInt => _myField; // it is not a getter for the field too |
There was a problem hiding this comment.
I think in this case it should report the lint since it is a getter for the field.
There was a problem hiding this comment.
yes, but I think it is necessary to additionally check whether there is another getter or variable with the same name.
There was a problem hiding this comment.
I don't think it make sense to search for a getter with exactly the same name as a variable. If we have a private variable and a public getter(with any name) that returns this variable we should show lint. Also, I think it makes sense to highlight the getter, but not the variable, so the user can see why we are saying that this field should be just public.
There was a problem hiding this comment.
Almost there, @4akloon! I've added one more suggestion and wanted to ask you to update the changelog with all the changes you've made so far.
| isGetter: true, | ||
| declaredElement: ExecutableElement( | ||
| isAbstract: false, | ||
| isStatic: false, |
There was a problem hiding this comment.
Do we actually need to skip static getters? I think we may have the same situation when a static getter returns the static final private variable that either does not make any sense. I would remove this restriction.
…erty from declaredElement
| class Fail2 { | ||
| final int _myField = 0; | ||
|
|
||
| // expect_lint: avoid_final_with_getter | ||
| int get myFieldInt => _myField; | ||
| } |
There was a problem hiding this comment.
Does it make sense to add a test for the static field and getter?
added new avoid_final_with_getter rule