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.

Sign up for GitHub

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

Jump to bottom

LevelFilter documents its comparison logic as the opposite of reality #1669

Closed
lilyball opened this issue Oct 21, 2021 · 5 comments · Fixed by #1693
Closed

LevelFilter documents its comparison logic as the opposite of reality #1669

lilyball opened this issue Oct 21, 2021 · 5 comments · Fixed by #1693
Assignees
@davidbarsky
Labels
crate/core Related to the `tracing-core` crate kind/docs

Comments

@lilyball
Copy link
Contributor

Bug Report

Version

Current published tracing-core and tracing, as well as the version currently documented on tracing.rs.

Crates

tracing-core

Description

The documentation for LevelFilter states

If a Level is considered less than a LevelFilter, it should be considered disabled; if greater than or equal to the LevelFilter, that level is enabled.

This is the exact opposite of how it works. LevelFilter compares the same way Level does, where ERROR is the lowest level and TRACE is the highest, with the addition of OFF as comparing less than ERROR. So the intended logic here is "if a Level is less than or equal to a LevelFilter it should be considered enabled; if greater than the LevelFilter, that level is disabled". Thankfully the LevelFilter::current() documentation is correct.

It would also be lovely to have a code sample added that demonstrates comparing a level against LevelFilter::current(), to help demonstrate how such comparisons work.

Side note: the LevelFilter::current() docs have a stray ` at the end.
@hawkw hawkw added crate/core Related to the `tracing-core` crate kind/docs labels Oct 26, 2021
@hawkw
Copy link
Member

hawkw commented Oct 26, 2021

@davidbarsky, since you love working on docs so much, mind fixing this? :)

@davidbarsky
Copy link
Member

@hawkw I'll take care of it today!

davidbarsky added a commit that referenced this issue Oct 27, 2021
@davidbarsky
docs: fix incorrect docs in tracing_core::LevelFilter; resolves #1669.
dd03113
@davidbarsky davidbarsky mentioned this issue Oct 27, 2021
Merged
hawkw pushed a commit that referenced this issue Oct 28, 2021
@davidbarsky
core: fix incorrect docs in tracing_core::LevelFilter (#1692)
3bff502 
Resolves #1669. Namely, I:
- fixed the incorrect docs on `LevelFilter`.
- Removed a stray backtick on `LevelFilter::current`.
- Added a matching backtick in Level's documentation.

I did _not_ add example of comparing a level against a `LevelFilter`; I
instead pointed readers to Level's documentation.
davidbarsky added a commit that referenced this issue Oct 29, 2021
@davidbarsky
core: fix incorrect docs in tracing_core::LevelFilter (#1692)
a3f24b3 
Resolves #1669. Namely, I:
- fixed the incorrect docs on `LevelFilter`.
- Removed a stray backtick on `LevelFilter::current`.
- Added a matching backtick in Level's documentation.

I did _not_ add example of comparing a level against a `LevelFilter`; I
instead pointed readers to Level's documentation.
@davidbarsky davidbarsky mentioned this issue Oct 29, 2021
Merged
hawkw pushed a commit that referenced this issue Oct 29, 2021
@davidbarsky @hawkw
core: fix incorrect docs in tracing_core::LevelFilter (#1692)
1d9b931 
Resolves #1669. Namely, I:
- fixed the incorrect docs on `LevelFilter`.
- Removed a stray backtick on `LevelFilter::current`.
- Added a matching backtick in Level's documentation.

I did _not_ add example of comparing a level against a `LevelFilter`; I
instead pointed readers to Level's documentation.
This was referenced Nov 4, 2022
Closed
Closed
@j4nk3e
Copy link

j4nk3e commented Nov 4, 2022

I noticed that this was probably not intentional, due to a mistake in the underlying comparison functions (see #2367). Wouldn't it be better to fix the implementation than to adjust the documentation to the unexpected behavior?

@davidbarsky
Copy link
Member

@juumixx (responding here for any readers who might be following along)

I explained why this is intentional on #2367 (comment). The documentation issue was unfortunate, but that's because there are several moving parts.

@j4nk3e
Copy link

j4nk3e commented Nov 4, 2022

Thanks, I guess whoever wrote the documentation was likewise confused by the behavior as I was.

maddiemort added a commit to maddiemort/tracing that referenced this issue Oct 19, 2023
@maddiemort
core: fix incorrect (incorrectly updated) docs for LevelFilter
94b7266 
These docs were updated (in response to tokio-rs#1669), but the "or equal to"
phrase should have been moved to the other case.

Fixes: tokio-rs#1993
@maddiemort maddiemort mentioned this issue Oct 19, 2023
Merged
hawkw pushed a commit that referenced this issue Oct 19, 2023
@maddiemort
core: fix incorrect (incorrectly updated) docs for LevelFilter (#2767)
90ef8e3 
These docs were updated (in response to #1669), but the "or equal to"
phrase should have been moved to the other case.

Fixes: #1993

## Motivation

When these docs were updated in response to #1669, they were updated
incompletely (a level that is equal to a filter is _enabled_, not
_disabled_) and therefore remained incorrect.

## Solution

The docs were updated, moving the "or equal to" phrase to the other
case.
davidbarsky pushed a commit that referenced this issue Nov 7, 2023
@maddiemort @davidbarsky
core: fix incorrect (incorrectly updated) docs for LevelFilter (#2767)
0b7371d 
These docs were updated (in response to #1669), but the "or equal to"
phrase should have been moved to the other case.

Fixes: #1993

## Motivation

When these docs were updated in response to #1669, they were updated
incompletely (a level that is equal to a filter is _enabled_, not
_disabled_) and therefore remained incorrect.

## Solution

The docs were updated, moving the "or equal to" phrase to the other
case.
hawkw pushed a commit that referenced this issue Nov 7, 2023
@maddiemort @hawkw
core: fix incorrect (incorrectly updated) docs for LevelFilter (#2767)
346d6e6 
These docs were updated (in response to #1669), but the "or equal to"
phrase should have been moved to the other case.

Fixes: #1993

## Motivation

When these docs were updated in response to #1669, they were updated
incompletely (a level that is equal to a filter is _enabled_, not
_disabled_) and therefore remained incorrect.

## Solution

The docs were updated, moving the "or equal to" phrase to the other
case.
kaffarell pushed a commit to kaffarell/tracing that referenced this issue Nov 21, 2023
@maddiemort @kaffarell
core: fix incorrect (incorrectly updated) docs for LevelFilter (tokio…
caef040 
…-rs#2767)

These docs were updated (in response to tokio-rs#1669), but the "or equal to"
phrase should have been moved to the other case.

Fixes: tokio-rs#1993

## Motivation

When these docs were updated in response to tokio-rs#1669, they were updated
incompletely (a level that is equal to a filter is _enabled_, not
_disabled_) and therefore remained incorrect.

## Solution

The docs were updated, moving the "or equal to" phrase to the other
case.
kaffarell pushed a commit to kaffarell/tracing that referenced this issue Feb 14, 2024
@maddiemort @kaffarell
core: fix incorrect (incorrectly updated) docs for LevelFilter (tokio…
5a2fc1c 
…-rs#2767)

These docs were updated (in response to tokio-rs#1669), but the "or equal to"
phrase should have been moved to the other case.

Fixes: tokio-rs#1993

## Motivation

When these docs were updated in response to tokio-rs#1669, they were updated
incompletely (a level that is equal to a filter is _enabled_, not
_disabled_) and therefore remained incorrect.

## Solution

The docs were updated, moving the "or equal to" phrase to the other
case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@davidbarsky davidbarsky

Labels
crate/core Related to the `tracing-core` crate kind/docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore: forwardport #1692 tokio-rs/tracing
4 participants
@lilyball @davidbarsky @hawkw @j4nk3e