-
-
Notifications
You must be signed in to change notification settings - Fork 82
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 in unawaited_futures #26
Comments
Thanks for opening this issue and starting a discussion 👍 I enabled it in few bigger apps I have access to, to check if something has changed since I disabled the rule. I found 16/72 cases where the check was correct and an Most false positives are button presses that start async operations (intended) or fire and forget calls like analytics event tracking. Another reason this check is disabled, which I haven't documented, is that the I'm happy to add it as default, given proof that it is correct in ~80% of cases and enough demand from the community (Upvote the issue if you read this). For the time being, enable it manually for your project: include: package:lint/analysis_options.yaml
linter:
rules:
# Always await, intentionally not await
unawaited_futures: true Don't forget to also add the |
We could always just copy the unawait function from pedantic.
On the flip side I really don't see an issue with depending on a package
that you are trying to improve on.
I do however think you are asking the wrong question.
Whilst the false positive rate is a factor the most important questions is
how damaging are these types of errors.
I would suggest that they are highly significant and perhaps more
importantly extremely hard to find. I've been burnt quite badly by these
types of problems on multiple occasions.
I would opinion that these factors outweigh the slight pain of the false
positives.
…On Wed, 16 Dec 2020 at 13:02, Pascal Welsch ***@***.***> wrote:
Thanks for opening this issue and starting a discussion 👍
I enabled it in few bigger apps I have access to, to check if something
has changed since I disabled the rule. I found 16/72 cases where the check
was correct and an await was missing. 77% of the time it is a false
positive though.
Most false positives are button presses that start async operations
(intended) or fire and forget calls like analytics event tracking.
Another reason this check is disabled, which I haven't documented, is that
the unawaited function is part of the pedantic package. To use it, lint
would have to depend on pedantic and export it. It would be a bit weird
to depend on something this project replaces.
I'm happy to add it as default, given proof that it is correct in ~80% of
cases and enough demand from the community (Upvote the issue if you read
this). For the time being, enable it manually for your project:
include: package:lint/analysis_options.yaml
linter:
rules:
# Always await, intentionally not await
unawaited_futures: true
Don't forget to also add the pedantic dependency to actually be able to
use the unawaited function.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#26 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OFVGZENTE7FYMGJZZDSVAIKPANCNFSM4U5EHVPA>
.
|
Was this ever solved? Do we have to include lint and pendantic to get the unawaited() function? |
I let this issue sit for a while because the I then checked the linter code and discovered - to my surprise - that the check is not tied to the Please check the PR for the new |
I spent a lot of time exploring
|
As above the doco suggest this does produce false positive.
The converse issues is that diagnosing a missing await can be a very complex issue.
I personally am happy to wear the burden of having to unawait methods to avoid bugs especially bugs that are hard to solve.
I really think this should be part of the default package.
I just added this to a small project (6000 lines) and it fixed three bugs and returned no false positives.
The text was updated successfully, but these errors were encountered: