-
-
Notifications
You must be signed in to change notification settings - Fork 142
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
fix(console): lints trigger with non-async tasks #517
Conversation
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.
Looks good to me overall, although I think it might be nice to factor out the repeated logic into a method. What do you think?
tokio-console/src/warnings.rs
Outdated
// Don't fire warning for tasks that are not async | ||
if task.kind() == "block_on" || task.kind() == "blocking" { | ||
return Warning::Ok | ||
} |
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.
nit, take it or leave it: this seems like it could potentially be factored out into a method on Task
, so that we could instead just write:
// Don't fire warning for tasks that are not async | |
if task.kind() == "block_on" || task.kind() == "blocking" { | |
return Warning::Ok | |
} | |
// Don't fire warning for tasks that are not async | |
if task.is_blocking() { | |
return Warning::Ok | |
} |
yea, good suggestion. I can do that! |
91d5134
to
cb033d1
Compare
I made the change. Please, don't hesitate to ask for any other change (nit or anything). I really want to learn, so I really appreciate any kind of suggestion! :) |
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.
This looks good to me! I had one additional suggestion, but it's not a hard blocker --- I'm happy to merge this either way. Thank you!
66c3ac4
to
3ab6734
Compare
looks lovely, thank you! |
change existing lints to only trigger with tasks that are async; those are all except those with kind 'blocking' and 'block_on' this commit fixes issue tokio-rs#516
3ab6734
to
c8c3391
Compare
…scriber-v0.3.0 (#557) ## 🤖 New release * `tokio-console`: 0.1.10 -> 0.1.11 * `console-api`: 0.6.0 -> 0.7.0 * `console-subscriber`: 0.2.0 -> 0.3.0 ## `tokio-console` ## tokio-console-v0.1.11 - (2024-06-10) ### Added - Replace target column with kind column in tasks view ([#478](#478)) ([903d9fa](903d9fa)) - Add flags and configurations for warnings ([#493](#493)) ([174883f](174883f)) - Add `--allow` flag ([#513](#513)) ([8da7037](8da7037)) ### Documented - Add note about running on Windows ([#510](#510)) ([a0d20fd](a0d20fd)) ### Fixed - Ignore key release events ([#468](#468)) ([715713a](715713a)) - Accept only `file://`, `http://`, `https://` URI ([#486](#486)) ([031bddd](031bddd)) - Fix column sorting in resources tab ([#491](#491)) ([96c65bd](96c65bd)) - Only trigger lints on async tasks ([#517](#517)) ([4593222](4593222)) - Remove duplicate controls from async ops view ([#519](#519)) ([f28ba4a](f28ba4a)) - Add pretty format for 'last woken' time ([#529](#529)) ([ea11ad8](ea11ad8)) ## `console-api` ## console-api-v0.7.0 - (2024-06-10) ### <a id = "0.7.0-breaking"></a>Breaking Changes - **Bump tonic to 0.11 ([#547](#547 ([ef6816c](https://github.com/tokio-rs/console/commit/ef6816caa0fe84171105b513425506f25d3082af))<br />This is a breaking change for users of `console-api` and `console-subscriber`, as it changes the public `tonic` dependency to a semver-incompatible version. This breaks compatibility with `tonic` 0.10.x. ### Documented - Fix typo in proto ([#472](#472)) ([2dd3559](2dd3559)) ### Updated - [**breaking**](#0.7.0-breaking) Bump tonic to 0.11 ([#547](#547)) ([ef6816c](ef6816c)) ## `console-subscriber` ## console-subscriber-v0.3.0 - (2024-06-10) ### <a id = "0.3.0-breaking"></a>Breaking Changes - **Bump tonic to 0.11 ([#547](#547 ([ef6816c](https://github.com/tokio-rs/console/commit/ef6816caa0fe84171105b513425506f25d3082af))<br />This is a breaking change for users of `console-api` and `console-subscriber`, as it changes the public `tonic` dependency to a semver-incompatible version. This breaks compatibility with `tonic` 0.10.x. ### Added - Replace target column with kind column in tasks view ([#478](#478)) ([903d9fa](903d9fa)) - Reduce retention period to fit in max message size ([#503](#503)) ([bd3dd71](bd3dd71)) - Support grpc-web and add `grpc-web` feature ([#498](#498)) ([4150253](4150253)) ### Documented - Add a grpc-web app example ([#526](#526)) ([4af30f2](4af30f2)) - Fix typo in doc comment ([#543](#543)) ([ff22678](ff22678)) ### Fixed - Don't save poll_ops if no-one is receiving them ([#501](#501)) ([1656c79](1656c79)) - Ignore metadata that is not a span or event ([#554](#554)) ([852a977](852a977)) ### Updated - [**breaking**](#0.3.0-breaking) Bump tonic to 0.11 ([#547](#547)) ([ef6816c](ef6816c))
change existing lints to only trigger with tasks that are async; those are all except those with kind 'blocking' and 'block_on'
this commit fixes issue #516