Make format::Writer::new() public #2680
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A number of people (myself included) have expressed significant interest in having Writer::new() public for various reasons, with some difficulty getting traction.
Kaifastromai public writer new
kaifastromai
requested review from
hawkw,
davidbarsky and
a team
as code owners
hawkw
approved these changes
Oct 10, 2023
Comment on lines
+426
to
+427
| //(@kaifastromai) I suppose having dedicated constructors may have certain benefits | ||
| // but I am not privy to the larger direction of tracing/subscriber. |
There was a problem hiding this comment.
If we're committing to making this a public API, I think we can just remove the previous TODO comment.
There was a problem hiding this comment.
The motivation for not making this public is that I thought it could be valuable for the Writer type to be able to indicate whether it was empty or not. This could be used when writing to it, in order to determine whether to emit whitespace before writing more text to the writer. If we were going to track this, however, we couldn't just take an arbitrary impl fmt::Write, because there is no way to determine whether that value is empty or not. If we had an explicit Writer::from_string constructor, instead, we could check if the string is empty first. Therefore, my preference would be to do that, and keep the new function that constructs a Writer from an arbitrary value private.
However, I honestly don't care anymore, and I'm happy to approve this PR regardless. If we decide that tracking whether the writer is empty is important, we could make a breaking change in the future.
|
Huzzah! |
hawkw
added a commit
that referenced
this pull request
Oct 12, 2023
## Motivation As seen here #2512 and #2223. Previously pr'ed here #2525, but no progress has been made there for quite some. I have applied the suggested changes. Not sure the formatting of the doc is sufficient or otherwise correct ## Solution Make the `format::Writer::new()` function public. I don't see any obvious trade-offs, but I am not familiar with the larger direction of tracing/subscriber, so I may be wrong. Closes #2512 Closes #2223 Co-authored-by: Cephas Storm <cephas.storm@borisfx.com> Co-authored-by: Eliza Weisman <eliza@buoyant.io>
davidbarsky
pushed a commit
that referenced
this pull request
Oct 12, 2023
## Motivation As seen here #2512 and #2223. Previously pr'ed here #2525, but no progress has been made there for quite some. I have applied the suggested changes. Not sure the formatting of the doc is sufficient or otherwise correct ## Solution Make the `format::Writer::new()` function public. I don't see any obvious trade-offs, but I am not familiar with the larger direction of tracing/subscriber, so I may be wrong. Closes #2512 Closes #2223 Co-authored-by: Cephas Storm <cephas.storm@borisfx.com> Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw
pushed a commit
that referenced
this pull request
Nov 13, 2023
# 0.3.18 (November 13, 2023) This release of `tracing-subscriber` adds support for the [`NO_COLOR`] environment variable (an informal standard to disable emitting ANSI color escape codes) in `fmt::Layer`, reintroduces support for the [`chrono`] crate, and increases the minimum supported Rust version (MSRV) to Rust 1.63.0. It also introduces several minor API improvements. ### Added - **chrono**: Add [`chrono`] implementations of `FormatTime` ([#2690]) - **subscriber**: Add support for the [`NO_COLOR`] environment variable in `fmt::Layer` ([#2647]) - **fmt**: make `format::Writer::new()` public ([#2680]) - **filter**: Implement `layer::Filter` for `Option<Filter>` ([#2407]) ### Changed - **log**: bump version of `tracing-log` to 0.2 ([#2772]) - Increased minimum supported Rust version (MSRV) to 1.63.0+. [`chrono`]: https://github.com/chronotope/chrono [`NO_COLOR`]: https://no-color.org/ [#2690]: #2690 [#2647]: #2647 [#2680]: #2680 [#2407]: #2407 [#2772]: #2772 Thanks to @shayne-fletcher, @dmlary, @kaifastromai, and @jsgf for contributing!
kaffarell
pushed a commit
to kaffarell/tracing
that referenced
this pull request
Nov 21, 2023
## Motivation As seen here tokio-rs#2512 and tokio-rs#2223. Previously pr'ed here tokio-rs#2525, but no progress has been made there for quite some. I have applied the suggested changes. Not sure the formatting of the doc is sufficient or otherwise correct ## Solution Make the `format::Writer::new()` function public. I don't see any obvious trade-offs, but I am not familiar with the larger direction of tracing/subscriber, so I may be wrong. Closes tokio-rs#2512 Closes tokio-rs#2223 Co-authored-by: Cephas Storm <cephas.storm@borisfx.com> Co-authored-by: Eliza Weisman <eliza@buoyant.io>
kaffarell
pushed a commit
to kaffarell/tracing
that referenced
this pull request
Feb 14, 2024
# 0.3.18 (November 13, 2023) This release of `tracing-subscriber` adds support for the [`NO_COLOR`] environment variable (an informal standard to disable emitting ANSI color escape codes) in `fmt::Layer`, reintroduces support for the [`chrono`] crate, and increases the minimum supported Rust version (MSRV) to Rust 1.63.0. It also introduces several minor API improvements. ### Added - **chrono**: Add [`chrono`] implementations of `FormatTime` ([tokio-rs#2690]) - **subscriber**: Add support for the [`NO_COLOR`] environment variable in `fmt::Layer` ([tokio-rs#2647]) - **fmt**: make `format::Writer::new()` public ([tokio-rs#2680]) - **filter**: Implement `layer::Filter` for `Option<Filter>` ([tokio-rs#2407]) ### Changed - **log**: bump version of `tracing-log` to 0.2 ([tokio-rs#2772]) - Increased minimum supported Rust version (MSRV) to 1.63.0+. [`chrono`]: https://github.com/chronotope/chrono [`NO_COLOR`]: https://no-color.org/ [tokio-rs#2690]: tokio-rs#2690 [tokio-rs#2647]: tokio-rs#2647 [tokio-rs#2680]: tokio-rs#2680 [tokio-rs#2407]: tokio-rs#2407 [tokio-rs#2772]: tokio-rs#2772 Thanks to @shayne-fletcher, @dmlary, @kaifastromai, and @jsgf for contributing!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
As seen here #2512 and #2223. Previously pr'ed here #2525, but no progress has been made there for quite some. I have applied the suggested changes. Not sure the formatting of the doc is sufficient or otherwise correct
Solution
Make the format::Writer::new() function public. I don't see any obvious trade-offs, but I am not familiar with the larger direction of tracing/subscriber, so I may be wrong.