Skip to content
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

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

javihernant
Copy link
Contributor

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

@javihernant javihernant requested a review from a team as a code owner February 7, 2024 17:37
Copy link
Member

@hawkw hawkw left a 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?

Comment on lines 151 to 154
// Don't fire warning for tasks that are not async
if task.kind() == "block_on" || task.kind() == "blocking" {
return Warning::Ok
}
Copy link
Member

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:

Suggested change
// 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
}

@javihernant
Copy link
Contributor Author

yea, good suggestion. I can do that!

@javihernant javihernant force-pushed the issue_516 branch 2 times, most recently from 91d5134 to cb033d1 Compare February 7, 2024 23:26
@javihernant
Copy link
Contributor Author

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! :)

Copy link
Member

@hawkw hawkw left a 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!

tokio-console/src/state/tasks.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Feb 8, 2024

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
@hawkw hawkw enabled auto-merge (squash) February 8, 2024 17:47
@hawkw hawkw merged commit 4593222 into tokio-rs:main Feb 8, 2024
11 checks passed
@javihernant javihernant deleted the issue_516 branch February 9, 2024 11:06
hds pushed a commit that referenced this pull request Jun 10, 2024
…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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants