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

subscriber: add ability to disable ANSI without crate feature #2532

Merged
merged 2 commits into from
Apr 14, 2023

Conversation

daxpedda
Copy link
Contributor

Motivation

Currently it is not possible to disable ANSI in fmt::Subscriber without enabling the ansi crate feature. This makes it difficult for users to implement interoperable settings that are controllable with crate features without having to pull in the dependencies ansi does.

I hit this while writing an application with multiple logging options set during compile-time and I wanted to cut down on dependencies if possible.

Solution

This adds a simple fmt::Subscriber::disable_ansi() that is available even without the ansi crate feature.

@daxpedda daxpedda requested review from hawkw, davidbarsky and a team as code owners March 26, 2023 18:28
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.

Hmm, I'm not sure if I like having an API with two separate ways to disable ANSI colors (with_ansi(false) and disable_ansi()), that seems a bit confusing for users. As an alternative solution, what do you think about changing with_ansi to no longer require the "ansi" feature flag, and having it print a warning (and possibly a debug_assert!) if the argument is true and the "ansi" feature flag is disabled?

@daxpedda
Copy link
Contributor Author

Hmm, I'm not sure if I like having an API with two separate ways to disable ANSI colors (with_ansi(false) and disable_ansi()), that seems a bit confusing for users.

I took this idea from reqwest btw: https://docs.rs/reqwest/0.11.16/reqwest/struct.ClientBuilder.html#method.no_brotli.

As an alternative solution, what do you think about changing with_ansi to no longer require the "ansi" feature flag, and havin

Sounds good to me!

@daxpedda
Copy link
Contributor Author

As an alternative solution, what do you think about changing with_ansi to no longer require the "ansi" feature flag, and having it print a warning (and possibly a debug_assert!) if the argument is true and the "ansi" feature flag is disabled?

Was just about to implement this when I realized I'm not sure how to "print" a warning here. Tracing isn't set up at this point yet, so should I be using eprintln!() then?

@hawkw
Copy link
Member

hawkw commented Apr 14, 2023

As an alternative solution, what do you think about changing with_ansi to no longer require the "ansi" feature flag, and having it print a warning (and possibly a debug_assert!) if the argument is true and the "ansi" feature flag is disabled?

Was just about to implement this when I realized I'm not sure how to "print" a warning here. Tracing isn't set up at this point yet, so should I be using eprintln!() then?

eprintln! is fine here.

Currently it is not possible to disable ANSI in `fmt::Subscriber`
without enabling the `ansi` crate feature. This makes it difficult for
users to implement interoperable settings that are controllable with
crate features without having to pull in the dependencies `ansi` does.

This removes the crate feature requirement on `fmt::Subscriber::with_ansi()`, but will print a warning if trying to enable ANSI terminal colors without the crate feature.
Comment on lines 279 to 287
#[cfg(not(feature = "ansi"))]
if ansi {
const ERROR: &str =
"tracing-subscriber: enabled ANSI terminal colors without the `ansi` crate feature";
#[cfg(debug_assertions)]
panic!("{}", ERROR);
#[cfg(not(debug_assertions))]
eprintln!("{}", ERROR);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope this is appropriate.
Should I add some documentation on with_ansi() about the warning? I didn't feel it's needed really.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should document that enabling ANSI formatting requires the feature flag. Now that the method itself is no longer feature gated, there's no obvious way to determine from the documentation that the feature flag is necessary.

@daxpedda daxpedda requested a review from hawkw April 14, 2023 16:35
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.

Overall, this looks great! I had some suggestions for rewording the documentation and the error message --- let me know what you think?

#[cfg(not(feature = "ansi"))]
if ansi {
const ERROR: &str =
"tracing-subscriber: enabled ANSI terminal colors without the `ansi` crate feature";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message describes what happened, rather than why it happened, which might be more helpful for the user. How about rephrasing it as something like this:

Suggested change
"tracing-subscriber: enabled ANSI terminal colors without the `ansi` crate feature";
"tracing-subscriber: the 'ansi' crate feature is required to enable ANSI terminal colors";

Copy link
Contributor Author

@daxpedda daxpedda Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's way better indeed.

I found that in other parts of the documentation in tracing are using ` instead of ' with crate features, so I used it here too. WDYT?

Comment on lines 279 to 287
#[cfg(not(feature = "ansi"))]
if ansi {
const ERROR: &str =
"tracing-subscriber: enabled ANSI terminal colors without the `ansi` crate feature";
#[cfg(debug_assertions)]
panic!("{}", ERROR);
#[cfg(not(debug_assertions))]
eprintln!("{}", ERROR);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should document that enabling ANSI formatting requires the feature flag. Now that the method itself is no longer feature gated, there's no obvious way to determine from the documentation that the feature flag is necessary.

tracing-subscriber/src/fmt/fmt_subscriber.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/fmt/mod.rs Outdated Show resolved Hide resolved
Co-Authored-By: Eliza Weisman <eliza@buoyant.io>
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 great to me!

@hawkw hawkw enabled auto-merge (squash) April 14, 2023 19:29
@hawkw hawkw merged commit 1cb523b into tokio-rs:master Apr 14, 2023
hawkw added a commit that referenced this pull request Apr 21, 2023
## Motivation

Currently it is not possible to disable ANSI in `fmt::Subscriber`
without enabling the "ansi" crate feature. This makes it difficult for
users to implement interoperable settings that are controllable with
crate features without having to pull in the dependencies "ansi" does.

I hit this while writing an application with multiple logging options
set during compile-time and I wanted to cut down on dependencies if
possible.

## Solution

This changes `fmt::Subscriber::with_ansi()` to not require the "ansi"
feature flag. This way, `with_ansi(false)` can be called even when the
"ansi" feature is disabled. Calling `with_ansi(true)` when the "ansi"
feature is not enabled will panic in debug mode, or print a warning if
debug assertions are disabled.

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Apr 21, 2023
## Motivation

Currently it is not possible to disable ANSI in `fmt::Subscriber`
without enabling the "ansi" crate feature. This makes it difficult for
users to implement interoperable settings that are controllable with
crate features without having to pull in the dependencies "ansi" does.

I hit this while writing an application with multiple logging options
set during compile-time and I wanted to cut down on dependencies if
possible.

## Solution

This changes `fmt::Subscriber::with_ansi()` to not require the "ansi"
feature flag. This way, `with_ansi(false)` can be called even when the
"ansi" feature is disabled. Calling `with_ansi(true)` when the "ansi"
feature is not enabled will panic in debug mode, or print a warning if
debug assertions are disabled.

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Apr 22, 2023
# 0.3.17 (April 21, 2023)

This release of `tracing-subscriber` fixes a build error when using
`env-filter` with recent versions of the `regex` crate. It also
introduces several minor API improvements.

### Fixed

- **env-filter**: Add "unicode-case" and "unicode-perl" to the `regex`
  dependency, fixing a build error with recent versions of `regex`
  (#2566)
- A number of minor documentation typos and other fixes (#2384, #2378,
  #2368, #2548)

### Added

- **filter**: Add `fmt::Display` impl for `filter::Targets` (#2343)
- **fmt**: Made `with_ansi(false)` no longer require the "ansi" feature,
  so that ANSI formatting escapes can be disabled without requiring
  ANSI-specific dependencies (#2532)

### Changed

- **fmt**: Dim targets in the `Compact` formatter, matching the default
  formatter (#2409)

Thanks to @keepsimple1, @andrewhalle, @LeoniePhiline, @LukeMathWalker,
@howardjohn, @daxpedda, and @dbidwell94 for contributing to this
release!
hongquan added a commit to hongquan/tracing that referenced this pull request Oct 10, 2023
This release of `tracing-subscriber` fixes a build error when using
`env-filter` with recent versions of the `regex` crate. It also
introduces several minor API improvements.

- **env-filter**: Add "unicode-case" and "unicode-perl" to the `regex`
  dependency, fixing a build error with recent versions of `regex`
  (tokio-rs#2566)
- A number of minor documentation typos and other fixes (tokio-rs#2384, tokio-rs#2378,
  tokio-rs#2368, tokio-rs#2548)

- **filter**: Add `fmt::Display` impl for `filter::Targets` (tokio-rs#2343)
- **fmt**: Made `with_ansi(false)` no longer require the "ansi" feature,
  so that ANSI formatting escapes can be disabled without requiring
  ANSI-specific dependencies (tokio-rs#2532)

- **fmt**: Dim targets in the `Compact` formatter, matching the default
  formatter (tokio-rs#2409)

Thanks to @keepsimple1, @andrewhalle, @LeoniePhiline, @LukeMathWalker,
@howardjohn, @daxpedda, and @dbidwell94 for contributing to this
release!
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
…rs#2532)

## Motivation

Currently it is not possible to disable ANSI in `fmt::Subscriber`
without enabling the "ansi" crate feature. This makes it difficult for
users to implement interoperable settings that are controllable with
crate features without having to pull in the dependencies "ansi" does.

I hit this while writing an application with multiple logging options
set during compile-time and I wanted to cut down on dependencies if
possible.

## Solution

This changes `fmt::Subscriber::with_ansi()` to not require the "ansi"
feature flag. This way, `with_ansi(false)` can be called even when the
"ansi" feature is disabled. Calling `with_ansi(true)` when the "ansi"
feature is not enabled will panic in debug mode, or print a warning if
debug assertions are disabled.

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.3.17 (April 21, 2023)

This release of `tracing-subscriber` fixes a build error when using
`env-filter` with recent versions of the `regex` crate. It also
introduces several minor API improvements.

### Fixed

- **env-filter**: Add "unicode-case" and "unicode-perl" to the `regex`
  dependency, fixing a build error with recent versions of `regex`
  (tokio-rs#2566)
- A number of minor documentation typos and other fixes (tokio-rs#2384, tokio-rs#2378,
  tokio-rs#2368, tokio-rs#2548)

### Added

- **filter**: Add `fmt::Display` impl for `filter::Targets` (tokio-rs#2343)
- **fmt**: Made `with_ansi(false)` no longer require the "ansi" feature,
  so that ANSI formatting escapes can be disabled without requiring
  ANSI-specific dependencies (tokio-rs#2532)

### Changed

- **fmt**: Dim targets in the `Compact` formatter, matching the default
  formatter (tokio-rs#2409)

Thanks to @keepsimple1, @andrewhalle, @LeoniePhiline, @LukeMathWalker,
@howardjohn, @daxpedda, and @dbidwell94 for contributing to this
release!
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