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

Expose Writer::new as public #2525

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

goodspark
Copy link

Solves #2512

I'm writing a custom formatting layer and want to use UtcTime and format_time to format the time into a string.

Solves tokio-rs#2512

I'm writing a custom formatting layer and want to use UtcTime and format_time to format the time into a string.
@goodspark goodspark requested review from hawkw, davidbarsky and a team as code owners March 21, 2023 18:14
// constructors instead (e.g. `from_string` that stores whether the string
// is empty...?)
pub(crate) fn new(writer: &'writer mut impl fmt::Write) -> Self {
pub fn new(writer: &'writer mut impl fmt::Write) -> Self {
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 going to make this a public API, it should have API documentation.

Also, since it's a constructor, let's add

Suggested change
pub fn new(writer: &'writer mut impl fmt::Write) -> Self {
#[must_use]
pub fn new(writer: &'writer mut impl fmt::Write) -> Self {

to ensure that the returned value is not accidentally discarded.

Copy link
Contributor

Choose a reason for hiding this comment

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

How does one add the documentation? I can do this if no one else has the time. I don't know too much about how this process works (just doc comments?), or whatever second and third degree affects might need to be documented.

@kaifastromai
Copy link
Contributor

Any update on this? I have this implemented in my own fork, but for some reason other crates that use a different version no longer work with the global subscriber, so getting this officially implemented would be great! I can make a new merge request with comments, but I'm not sure of all the considerations that need to be taken into account.

@kaifastromai
Copy link
Contributor

Any update on this?

@kayabaNerve
Copy link

Is there any way to yet again request eyes on this/the new PR (#2680)/the issue (#2512)? This minor change not being handled has blocked my ability to update :/ I do understand no one is under any obligation to spend their time working on this, but the new PR does resolve the comments here and seems to have been completely ignored. Hopefully putting in an extra ping is a valid reminder.

@hawkw
Copy link
Member

hawkw commented Oct 10, 2023

Is there any way to yet again request eyes on this/the new PR (#2680)/the issue (#2512)? This minor change not being handled has blocked my ability to update :/ I do understand no one is under any obligation to spend their time working on this, but the new PR does resolve the comments here and seems to have been completely ignored. Hopefully putting in an extra ping is a valid reminder.

Hi, I've reviewed the new PR, #2680. Hopefully we can get that merged soon.

Sorry I didn't address this sooner --- I'm more or less the only maintainer of all the tracing crates in this repo, and ongoing tracing maintenance is not one of my primary responsibilities for my employer, so I don't always have time to keep up with activity on PRs like this one, and sometimes they slip through the cracks. I don't mind the ping, and I'm sorry that this not being merged sooner has inconvenienced you!

hawkw added a commit that referenced this pull request Oct 11, 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 <[email protected]>
Co-authored-by: Eliza Weisman <[email protected]>
@kayabaNerve
Copy link

Don't worry at all! I totally get it. FOSS can be a busy mess. I appreciate all the work you do, just wanted to do what I can to help move things forward :)

Thanks for handling this!

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 <[email protected]>
Co-authored-by: Eliza Weisman <[email protected]>
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 <[email protected]>
Co-authored-by: Eliza Weisman <[email protected]>
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 <[email protected]>
Co-authored-by: Eliza Weisman <[email protected]>
@mladedav
Copy link
Contributor

This can be closed as #2680 has been merged.

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.

5 participants