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

Document panic and format edition/verions, and improve xrefs #96518

Closed
wants to merge 7 commits into from

Conversation

ijackson
Copy link
Contributor

This MR addresses three somewhat-related issues:

  • The primary stdlib docs for panic! did not document the 2021 edition differences.
  • The primary docs forformat! (in std::fmt) did not document the version requirement for implicit capture.
  • The primary docs for format! (in std::fmt) were badly referenced. Somehow every time I wanted them I ended up in format_args! or somewhere, clicking round in circles...

Having a section for this inspired by the docs for array::IntoIterator
For most things in the stdlib, there's the rustdoc-generated version
annotations.  I don't think we can do this here, so just write prose.
I have tried to be as terse as possible while still noting the
relevant gotchas.
That xref contains the actual documentation for what format! does.
It should be very prominent - particularly, more so than the other
links.
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 28, 2022
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @scottmcm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 28, 2022
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

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

Thanks for sending the PR! I like the approach of sending things consistently to the module documentation, rather than to the various macros.

Please address the various comments -- either with changes or with prose saying why you disagree -- then mark the PR as ready for review again with @rustbot ready.

@@ -84,6 +84,7 @@
//!
//! If a named parameter does not appear in the argument list, `format!` will
//! reference a variable with that name in the current scope.
//! (Rust 1.58 and later; in [`panic!`], requires Rust 2021.)
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this line. We don't include version numbers in the docs for every difference -- we host versioned copies of the docs (like https://doc.rust-lang.org/1.57.0/std/macro.format.html) so people using a particular version can see what that version supports. And we don't need to document panic!'s edition behaviour here -- that's fine just in its own documentation.

Suggested change
//! (Rust 1.58 and later; in [`panic!`], requires Rust 2021.)

@@ -64,6 +64,25 @@ For more detailed information about error handling check out the [book] or the
If the main thread panics it will terminate all your threads and end your
program with code `101`.

# Editions
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, having this written out here is really handy 👍

One request: Can you put the information about the newest edition first in this section? We want to optimize the docs for people using the latest edition, and have people still using older ones need to scan down to find their behaviour, rather than having people using 2021 need to hunt.

Maybe it could work well to make subsections here? Sketch,

# Editions

## 2021 and later

... `panic!` always uses formatting; use `panic_any` if you want a value ...

## 2018 and 2015

... panics with a payload of literally ...

@@ -9,7 +9,7 @@ tests. `panic!` is closely tied with the `unwrap` method of both
`panic!` when they are set to [`None`] or [`Err`] variants.

When using `panic!()` you can specify a string payload, that is built using
the [`format!`] syntax. That payload is used when injecting the panic into
the [`format!` syntax]. That payload is used when injecting the panic into
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: maybe just say

Suggested change
the [`format!` syntax]. That payload is used when injecting the panic into
the [formatting syntax]. That payload is used when injecting the panic into

rather that mentioning a particular macro? That avoids a conversation about whether it's "the format! syntax" or "the format_args! syntax" or ...

@scottmcm scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 13, 2022
@JohnCSimon
Copy link
Member

ping from triage:
@ijackson - can you address the build failures and comments from the reviewer? thank you

@JohnCSimon
Copy link
Member

@ijackson
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Jul 23, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jul 23, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 28, 2023
Document panic behavior across editions, and improve xrefs

This revives (parts of) rust-lang#96518.
r? `@scottmcm`
Cc `@ijackson`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants