-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Use modern formatting for format! macros #93950
Use modern formatting for format! macros #93950
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon. Please see the contribution instructions for more information. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Mixing positional and named format args looks quite puzzling, maybe not change that ones? |
@klensy good input, I think we should seek the maintainer's input on this (Mark-Simulacrum). If necessary, I'm willing to make the changes. |
Yeah, I think in general if there are positional arguments (e.g., because they use a more complex expression) we should avoid the usage of inline arguments for now. I'm not sure this is necessarily a good idea across the board, particularly in user-visible documentation, quite yet, either -- while we do only officially support 1 stable release, many Rust users are on somewhat older releases and identifiers just shipped in 1.58 not 6 weeks ago. I'll start a Zulip thread on this to see if there's opinions on this -- https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/format.20.7Bident.7D.20usage.20in.20docs @bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 2b057cada2f01a2f710468823173f4dbcea75229 with merge 545b01588be441368ca92a109eec76f75737baf1... |
☀️ Try build successful - checks-actions |
Queued 545b01588be441368ca92a109eec76f75737baf1 with parent b321742, future comparison URL. |
Finished benchmarking commit (545b01588be441368ca92a109eec76f75737baf1): comparison url. Summary: This benchmark run did not return any relevant results. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. @bors rollup=never |
This comment has been minimized.
This comment has been minimized.
@T-O-R-U-S I'm curious, while you were doing this, did you take note of what sorts of things couldn't be updated to the new syntax? What percentage of the format strings in the compiler could be successfully updated to use the new syntax? Would that percentage have been significantly increased if the format string syntax allowed trivial extensions like e.g. allowing struct field access? |
Unfortunately, no. I used a regex to quickly look for instances of old format strings in the codebase (of which there are far too many) and changed them by hand for fear of accidentally (and silently) breaking something. If I were to estimate, ~5% of inline format strings couldn't be done because struct field access wasn't possible. |
Should the documentation be left alone now, or should the rest of it be updated? @Mark-Simulacrum |
Hm, so given this would land at the earliest in 1.61, I think we can likely afford to update the documentation as well. Let's update the docs as well. If you can squash the commits after doing so as well, that'll help speed along a potential approval -- this sort of PR doesn't (at least for me) really benefit from lots of individual commits. |
This comment has been minimized.
This comment has been minimized.
Can you confirm that this updates all such cases across the compiler/standard library? I'd prefer to keep this to just 1-2 commits over time, to minimize noise in git blame and such. |
Hm, so I think limiting it to just library/ (and documentation, at that) is probably fine -- updating to the new format throughout the compiler if there's that many cases is probably more noisy than helpful and just causes git history churn for relatively little benefit. It can be patched up as those parts of the codebase naturally change over time. |
On that topic, looks like I forgot to go through |
There was a flaw with my regex and it has now become apparent to me that To elaborate, the regex didn't account for things like |
~500 un-updated format strings remain in |
I think The Book would need more delicate handling. It appears that Rust by Example is already partially updated to support the new syntax, but to use it in the Book might confuse users, and I also believe that extending the book to include a section about the new syntax is out of the scope of this PR. |
Let's avoid bumping submodules in that PR, it'll increase the amount of conflicts and generally cause more pain -- they can get bumped on their normal schedules. I agree that most of the book-like documentation likely is not as easy to update and should be left out of scope. |
3f1e116
to
815d680
Compare
@bors r+ rollup=iffy Squashed commits down and rebased. |
📌 Commit 815d680d736a87db4428b93a7f0d6fce937cc5f8 has been approved by |
☔ The latest upstream changes (presumably #94734) made this pull request unmergeable. Please resolve the merge conflicts. |
fa5e5cb
to
7730980
Compare
☔ The latest upstream changes (presumably #94802) made this pull request unmergeable. Please resolve the merge conflicts. |
This updates the standard library's documentation to use the new syntax. The documentation is worthwhile to update as it should be more idiomatic (particularly for features like this, which are nice for users to get acquainted with). The general codebase is likely more hassle than benefit to update: it'll hurt git blame, and generally updates can be done by folks updating the code if (and when) that makes things more readable with the new format. A few places in the compiler and library code are updated (mostly just due to already having been done when this commit was first authored).
7730980
to
72a25d0
Compare
@bors r+ Squashed commits down and rebased. |
📌 Commit 72a25d0 has been approved by |
Rollup of 7 pull requests Successful merges: - rust-lang#93950 (Use modern formatting for format! macros) - rust-lang#94274 (Treat unstable lints as unknown) - rust-lang#94368 ([1/2] Implement macro meta-variable expressions) - rust-lang#94719 (Statically compile libstdc++ everywhere if asked) - rust-lang#94728 (Only emit pointer-like metadata for `Box<T, A>` when `A` is ZST) - rust-lang#94790 (enable portable-simd doctests in Miri) - rust-lang#94811 (Update browser-ui-test version) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This updates the standard library's documentation to use the new format_args syntax.
The documentation is worthwhile to update as it should be more idiomatic
(particularly for features like this, which are nice for users to get acquainted
with). The general codebase is likely more hassle than benefit to update: it'll
hurt git blame, and generally updates can be done by folks updating the code if
(and when) that makes things more readable with the new format.
A few places in the compiler and library code are updated (mostly just due to
already having been done when this commit was first authored).
eprintln!("{}", e)
becomeseprintln!("{e}")
, buteprintln!("{}", e.kind())
remains untouched.