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

Itertools::format[_with] docs mention panic within logging macro #941

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Philippe-Cholet
Copy link
Member

@Philippe-Cholet Philippe-Cholet commented May 17, 2024

Closes #667 and maybe #307.

I merely update the docs of Itertools::format[_with], reformulations are welcome:

⚠ This can happen discreetly and be hard to debug if used in macros of some logging frameworks like tracing! ⚠

Copy link

codecov bot commented May 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.45%. Comparing base (6814180) to head (df96c73).
Report is 107 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #941      +/-   ##
==========================================
+ Coverage   94.38%   94.45%   +0.06%     
==========================================
  Files          48       49       +1     
  Lines        6665     7086     +421     
==========================================
+ Hits         6291     6693     +402     
- Misses        374      393      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Philippe-Cholet
Copy link
Member Author

@Veetaha Does it seem enough to you?

Copy link

@Veetaha Veetaha left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

src/lib.rs Outdated Show resolved Hide resolved
@Philippe-Cholet
Copy link
Member Author

Philippe-Cholet commented May 21, 2024

@phimuemue CI of recent #943 was unexpectedly long too (20 minutes more) and I don't know why.
This time it's more than 30 minutes.

@phimuemue
Copy link
Member

Did we ever think about requiring Clone for format to avoid the panic-problem?

@phimuemue
Copy link
Member

@phimuemue CI of recent #943 was unexpectedly long too (20 minutes more) and I don't know why. This time it's more than 30 minutes.

No idea why CI takes so long. I skimmed the individual steps, and they seem to add up to something around 3 minutes. Maybe an infrastructure issue on Github's side?

@Philippe-Cholet
Copy link
Member Author

#307 mentions adding a Clone bound as a possibility but I think that it would be a nasty (not easy to fix) breaking change to do now.

About CI, I don't know what it is. I looked at the log, saw a link about curl 8.0 (in rust 1.71) having time issues but it does not seem relevant to us (being at rust 1.78).
Currently, I hope it basically resolves itself, we are not doing anything special. Maybe a github action will have an update that fix this or the GitHub infrastructure will be somehow fixed shortly.

src/lib.rs Outdated
Comment on lines 2321 to 2323
/// **Panics** if the formatter helper is formatted more than once.
/// ⚠ This can happen unexpectedly and be hard to debug if used in
/// _macros of some logging frameworks_ like `tracing`! ⚠
Copy link
Member

Choose a reason for hiding this comment

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

A few requests:

  • can you give **Panics** its own # Header?
  • can your provide a should_panic doctest example of when this panics
    (don't add tracing as a dev-dependency though, since it has a different MSRV policy)
  • can you investigate replacing with rustdoc's experimental support for first-class warning blocks?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. I can give it its own header.
  2. I can simulate a tracing macro, and hide it by commenting it out.
  3. About replacing , we can't use markdown _words_ `tracing`! inside <div class="warning"></div> so either full html <i>words</i> <code>tracing</code> or keep it as is, your call.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jswrenn Done. I can dump the second commit if you finally prefer .

…ng macros

I failed to make the (invisible) macro usable as `tracing::info!` with some private module. Renamed it `tracing_info` instead.
I add `should_panic` but checked it panics for the right reason.
Markdown can not be used inside it.
It renders well in the documentation and in my IDE (Sublime Text: orange text).
Copy link
Member

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

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

A final nit, and then I'm happy to approve!

/// When the formatter helper is formatted more than once.
///
/// <div class="warning">This can happen unexpectedly and be hard to debug if used in
/// <i>macros of some logging frameworks</i> like <code>tracing</code>!</div>
Copy link
Member

Choose a reason for hiding this comment

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

Instead of <code>tracing</code>, let's link to https://docs.rs/tracing

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.

Revisit panicking in std::fmt::Display implementations as a footgun
4 participants