[riverpod_lint] Add new lint incorrect_usage_of_ref_method #2947
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2947 +/- ##
==========================================
+ Coverage 95.24% 95.29% +0.04%
==========================================
Files 53 53
Lines 2252 2274 +22
==========================================
+ Hits 2145 2167 +22
Misses 107 107
|
|
Hello! I think we should have one lint per ref method. So folks can enable/disable what they wish |
packages/riverpod_lint/README.md
Outdated
| }); | ||
|
|
||
| class MyNotifier extends Notifier<int> { | ||
| int get _watchGetter => ref.watch(provider); // use read instead |
There was a problem hiding this comment.
I don't recommend making such getters at all. They can introduce bugs implicitly, because the getter may be used in multiple places where some places want read and others want watch
There was a problem hiding this comment.
I see. I will remove it.
packages/riverpod_lint/README.md
Outdated
|
|
||
| Widget build(ctx, ref) { | ||
| ref.read(provider); // use watch instead | ||
| ref.listenManual(provider, ...); // use listen instead |
There was a problem hiding this comment.
There's no strong requirement against this ... as long as they use the returned subscription and deal with it.
There was a problem hiding this comment.
For instance they could do
sub?.cancel();
sub = ref.listenManual(...)
packages/riverpod_lint/README.md
Outdated
|
|
||
| void someMethod() { | ||
| ref.watch(provider); // use read instead | ||
| ref.listen(provider, ...); // place listen in build instead |
There was a problem hiding this comment.
The recommendation would be to use listenManual
There was a problem hiding this comment.
but this is inside a Notifier, ref is not WidgetRef.
There was a problem hiding this comment.
Sorry I missed that. The there's no issue with calling listen in methods.
As long as the return value is used to cancel subscription.
There was a problem hiding this comment.
I see. So this is a valid use case of Ref.listen.
void someMethod(){
sub?.close();
sub = ref.listen(provider, ...);
}
|
One important case to handle for build(context, ref) {
return ListView.builder(
builder: (context, index) {
ref.watch(...); // This is fine
}
);
}This shouldn't be allowed for |
|
I would also dissociate the lints based on which ref they are coming from. |
No problem. I could refactor it to multiple lints. |
Yeah. I am already doing that, e.g. using |
By that I meant having a different lint |
… ref.watch inside Consumer/HookConsumer
|
hmm... the lint is harder than I thought. |
Try to close #2445.
This lint turns out to be more challenging than I thought. Although It may not cover all the edge cases, I hope it will still be a good starting point to build upon.