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

Make format::Writer::new() public #2680

Merged
merged 5 commits into from
Oct 11, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion tracing-subscriber/src/fmt/format/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,19 @@ impl<'writer> Writer<'writer> {
// We may not want to do that if we choose to expose specialized
// constructors instead (e.g. `from_string` that stores whether the string
// is empty...?)
pub(crate) fn new(writer: &'writer mut impl fmt::Write) -> Self {
//(@kaifastromai) I suppose having dedicated constructors may have certain benefits
// but I am not privy to the larger direction of tracing/subscriber.
Comment on lines +425 to +426
Copy link
Member

Choose a reason for hiding this comment

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

If we're committing to making this a public API, I think we can just remove the previous TODO comment.

Copy link
Member

Choose a reason for hiding this comment

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

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.

/// Create a new [`Writer`] from any type that implements [`fmt::Write`].
///
/// The returned `Writer` value may be passed as an argument to methods
/// such as [`Format::format_event`]. Since constructing a `Writer`
/// mutably borrows the underlying [`fmt::Write`] instance, that value may
/// be accessed again once the `Writer` is dropped. For example, if the
/// value implementing [`fmt::Write`] is a [`String`], it will contain
/// the formatted output of [`Format::format_event`], which may then be
/// used for other purposes.
#[must_use]
pub fn new(writer: &'writer mut impl fmt::Write) -> Self {
Self {
writer: writer as &mut dyn fmt::Write,
is_ansi: false,
Expand Down