Fix spacing of links in inline code. #88343

steffahn · 2021-08-25T20:09:47Z

Similar to #80733, but the focus is different. This PR eliminates all occurrences of pieced-together inline code blocks like Box<Option<T>> and replaces them with good-looking ones (using HTML-syntax), like Box<Option<T>>. As far as I can tell, I should’ve found all of these in the standard library (regex search with r"`\]`|`\[`") [except for in core::convert where I’ve noticed other things in the docs that I want to fix in a separate PR]. In particular, unlike #80733, I’ve added almost no new instance of inline code that’s broken up into multiple links (or some link and some link-free part). I also added tooltips (the stuff in quotes for the markdown link listings) in places that caught my eye, but that’s by no means systematic, just opportunistic.

Context: I got annoyed by repeatedly running into new misformatted inline code while reading the standard library docs. I know that once issue #83997 (and/or related ones) are resolved, these changes become somewhat obsolete, but I fail to notice much progress on that end right now.

r? @jyn514

steffahn · 2021-08-25T21:08:30Z

TIL, tools/linkchecker is a thing. Won't be able to fix until tomorrow (~11h).

steffahn · 2021-08-26T08:28:03Z

Should hopefully pass CI now

jyn514 · 2021-08-26T16:04:51Z

That was some very accurate math 😆

jyn514 · 2021-08-26T16:05:36Z

Seems fine in principle but I haven't read the diff, I'll try to get to it this weekend.

jyn514

Looked at the first couple commits and they seem fine, but there's a lot of churn adding tooltips and I'm not really sure why they're useful to add - would like to hear more about that before spending more time on review.

jyn514 · 2021-09-06T20:16:00Z

library/alloc/src/fmt.rs

-//! [`format!`]: crate::format
-//! [`to_string`]: crate::string::ToString
-//! [`writeln!`]: core::writeln
+//! [`fmt::Result`]: Result "fmt::Result"


By default this won't have a tooltip, right? Why did you add one?

In general, links generated by rustdoc do have a tooltip.

E.g. something like

/// Talking about [`Result`][std::fmt::Result] here! pub mod m {}

will come with a tooltip

Whereas (unfortunately) the following alternative

/// Talking about [`Result`] here! /// /// [`Result`]: std::fmt::Result pub mod m {}

creates almost the same documentation, just lacking the tooltip. That’s the general reason why I’ve converted many instances of

[foo]: path

into

[foo]: path "path"

and in some cases, I’ve also adjusted the tooltip slightly to make it more easy to understand. This includes the case here because Result is part of the prelude, a simple "Result" tooltip seems confusing to me, the std:: prefix is something I’ve often avoided for tooltips OTOH (not least because the paths themselves also often are relative so some imported std::foo module).

Also, as mentioned in the description of this PR, the addition of tooltips isn’t systematic and only covers places that I happened to come across. (Furthermore, the amount of tooltips added declines from earlier to later commits.)

Ok. I think this would be better to fix systematically in rustdoc rather than adding it piecemeal to individual links. Could you open an issue and revert the tooltips you've added this PR?

"revert" as in “add another commit to revert them” or as a “rebase / edit / force push”. Because the former could easily happen in a later PR, too; these tooltips aren't particularly hard to find (and remove) with a regex search.

Either way, the revert would only remove those cases where the path and tooltip look identical, right? E.g. the fmt::Result should better stay IMO, because I don't like a Result tooltip.

Hmm, no answer. I’m assuming that an additional commit is okay, and that only the ones where tooltip and path are identical need to be removed. This is more easily accomplished by a systematic find-and-replace that by looking through all my commits here. This could, as noted, easily be a separate PR, done after the relevant change to rustdoc was made, but I’m including a commit here now to get this PR going.

Sorry for the delay, I haven't had much time for rust lately.

Either way, the revert would only remove those cases where the path and tooltip look identical, right? E.g. the fmt::Result should better stay IMO, because I don't like a Result tooltip.

Well, ideally I wouldn't want any changes, because it makes them inconsistent and it will hopefully be done by rustdoc automatically eventually ... but if you feel strongly it seems fine to keep, it's just more work for me to review. I misunderstood - you're talking about showing core::result::Result links as std::result::Resultand that sort of thing. I guess that's fine? I think showing the link used in the source is more clear in a way but I don't feel strongly.

I’m assuming that an additional commit is okay, and that only the ones where tooltip and path are identical need to be removed.

Yup, that's fine - I don't have strong opinions about git commits.

I think showing the link used in the source is more clear in a way but I don't feel strongly.

The problem I see is that the link used in the source code depends on what the imports are, and the imports depends on what happens to be used in the source code. If e.g. I’m writing documentation mentioning, say, mem::transmute, then I’d use the path men::transmute (resulting in a men::transmute tooltip) if the module I’m in does use core::mem (or use crate::mem). But if mem is only used in docs, AFAIK I can’t just add an import for that because (I think) it’ll trigger an unused imports lint and fail CI. But it’s IMO better to concistently have it have the tooltip mem::transmute and not sometimes core::mem::transmute or and crate::mem::transmute, etc…, depending on the implementation details (i.e. imports) of the module your documentation comes from (in particular, I don’t think the crate:: prefix is a good thing showing up in standard library tooltips).

steffahn · 2021-09-06T20:50:16Z

I'm not really sure why they're useful to add

For me it’s mostly a question of consistency. I don’t like not having a tooltip simply because of the decision to not put the link inline but separately in a links section in markdown. I don’t like reading docs where half the links give me tooltips and the other half don’t (for no apparent reason to the reader); that’s also the reason why I had systematically included tooltips in #80733.

steffahn · 2021-09-23T11:59:04Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

jyn514

I would prefer not to review PRs this big in the future - please wait until some of the rustdoc issues are fixed before making more PRs.

jyn514 · 2021-09-25T15:34:07Z

library/alloc/src/string.rs

+/// [`str`]: prim@str "str"
+/// [`&str`]: prim@str "&str"
+/// [Deref]: core::ops::Deref "ops::Deref"
+/// [`Deref`]: core::ops::Deref "ops::Deref"


nit: you can avoid needing a second reference link by using [`Deref`][Deref]. But doesn't seem important to change.

jyn514 · 2021-09-25T15:35:04Z

library/alloc/src/sync.rs

@@ -476,7 +477,7 @@ impl<T> Arc<T> {
    /// assert_eq!(*zero, 0)
    /// ```
    ///
-    /// [zeroed]: ../../std/mem/union.MaybeUninit.html#method.zeroed
+    /// [zeroed]: mem::MaybeUninit::zeroed


Huh, did this move into core recently or something? Or was it just missed when converting links to intra-doc links?

I have no idea :-)

library/core/src/option.rs

jyn514 · 2021-09-25T17:01:46Z

r=me with the linkchecker passing. Not sure if you have bors privileges or not, just in case -

@bors delegate=steffahn

Please use r=jyn514, not r+.

bors · 2021-09-25T17:01:48Z

✌️ @steffahn can now approve this pull request

jyn514 · 2021-09-25T17:02:12Z

oh - and please squash the commits before merging too if you don't mind :)

steffahn · 2021-09-25T17:02:44Z

Not sure if you have bors privileges

I don’t :-)

please squash the commits before merging

okay

---------- Fix spacing for links inside code blocks, and improve link tooltips in alloc::fmt ---------- Fix spacing for links inside code blocks, and improve link tooltips in alloc::{rc, sync} ---------- Fix spacing for links inside code blocks, and improve link tooltips in alloc::string ---------- Fix spacing for links inside code blocks in alloc::vec ---------- Fix spacing for links inside code blocks in core::option ---------- Fix spacing for links inside code blocks, and improve a few link tooltips in core::result ---------- Fix spacing for links inside code blocks in core::{iter::{self, iterator}, stream::stream, poll} ---------- Fix spacing for links inside code blocks, and improve a few link tooltips in std::{fs, path} ---------- Fix spacing for links inside code blocks in std::{collections, time} ---------- Fix spacing for links inside code blocks in and make formatting of `&str`-like types consistent in std::ffi::{c_str, os_str} ---------- Fix spacing for links inside code blocks, and improve link tooltips in std::ffi ---------- Fix spacing for links inside code blocks, and improve a few link tooltips in std::{io::{self, buffered::{bufreader, bufwriter}, cursor, util}, net::{self, addr}} ---------- Fix typo in link to `into` for `OsString` docs ---------- Remove tooltips that will probably become redundant in the future ---------- Apply suggestions from code review Replacing `…std/primitive.reference.html` paths with just `reference` Co-authored-by: Joshua Nelson <github@jyn.dev> ---------- Also replace `…std/primitive.reference.html` paths with just `reference` in `core::pin`

steffahn · 2021-09-25T18:12:28Z

Squashed (no changes)

@bors r=jyn514

bors · 2021-09-25T18:12:30Z

📌 Commit 67065fe has been approved by jyn514

bors · 2021-09-25T20:08:15Z

⌛ Testing commit 67065fe with merge addb4da...

bors · 2021-09-25T23:24:30Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing addb4da to master...

rust-timer · 2021-09-26T00:57:16Z

Finished benchmarking commit (addb4da): comparison url.

Summary: This change led to large relevant regressions 😿 in compiler performance.

Large regression in instruction counts (up to 2.1% on incr-unchanged builds of webrender-wrench)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

steffahn · 2021-09-26T01:14:17Z

This “regression” is about how fast rustc compiles some specific crate, right? (I have no idea where to find more information about those test cases.) This PR only changes (doc) comments in the standard library, so the regression is either sporadic or very weird.

If it's a sporadic thing, the benchmark of the next PR that gets merged by bors should perform better again; we'll see.

rylev · 2021-10-01T10:07:38Z

An update: Turns out this was a "very weird" regression. Thanks to great investigation by @Mark-Simulacrum, we've identified a possible cause for this which should be fixed by #89408. I'm going to mark this as triaged.

@rustbot label +perf-regression-triaged

@steffahn just as an FYI, you can find more info on perf testing here.

rust-highfive assigned jyn514 Aug 25, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2021

This comment has been minimized.

Sign in to view

jyn514 added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 26, 2021

jyn514 reviewed Sep 6, 2021

View reviewed changes

jyn514 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 Sep 6, 2021

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 23, 2021

jyn514 mentioned this pull request Sep 25, 2021

Rustdoc does not add tooltips for reference links #89246

Closed

jyn514 reviewed Sep 25, 2021

View reviewed changes

jyn514 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 Sep 25, 2021

steffahn force-pushed the fix_code_spacing branch from 0cd3835 to 67065fe Compare September 25, 2021 18:09

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 25, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 25, 2021

bors merged commit addb4da into rust-lang:master Sep 25, 2021

rustbot added this to the 1.57.0 milestone Sep 25, 2021

bors mentioned this pull request Sep 25, 2021

Rewrite Pin<P> docs to clarify guarantees and uses #88500

Closed

rustbot added the perf-regression Performance regression. label Sep 26, 2021

rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 1, 2021

Nemo157 mentioned this pull request Sep 12, 2022

Change default level of INVALID_HTML_TAGS to warning and stabilize it #101720

Merged

steffahn deleted the fix_code_spacing branch January 19, 2023 15:30

gurry mentioned this pull request Dec 3, 2023

ice: ProjectionKinds Deref and Field were mismatched #118144

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix spacing of links in inline code. #88343

Fix spacing of links in inline code. #88343

steffahn commented Aug 25, 2021 •

edited

Loading

This comment has been minimized.

steffahn commented Aug 25, 2021

steffahn commented Aug 26, 2021

jyn514 commented Aug 26, 2021

jyn514 commented Aug 26, 2021

jyn514 left a comment

jyn514 Sep 6, 2021

steffahn Sep 6, 2021

jyn514 Sep 6, 2021

steffahn Sep 6, 2021

steffahn Sep 23, 2021

jyn514 Sep 25, 2021 •

edited

Loading

steffahn Sep 25, 2021

steffahn commented Sep 6, 2021 •

edited

Loading

steffahn commented Sep 23, 2021

jyn514 left a comment

jyn514 Sep 25, 2021 •

edited

Loading

jyn514 Sep 25, 2021

steffahn Sep 25, 2021

jyn514 commented Sep 25, 2021

bors commented Sep 25, 2021

jyn514 commented Sep 25, 2021

steffahn commented Sep 25, 2021

steffahn commented Sep 25, 2021

bors commented Sep 25, 2021

bors commented Sep 25, 2021

bors commented Sep 25, 2021

rust-timer commented Sep 26, 2021

steffahn commented Sep 26, 2021

rylev commented Oct 1, 2021 •

edited

Loading

Fix spacing of links in inline code. #88343

Fix spacing of links in inline code. #88343

Conversation

steffahn commented Aug 25, 2021 • edited Loading

This comment has been minimized.

steffahn commented Aug 25, 2021

steffahn commented Aug 26, 2021

jyn514 commented Aug 26, 2021

jyn514 commented Aug 26, 2021

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steffahn commented Sep 6, 2021 • edited Loading

steffahn commented Sep 23, 2021

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 commented Sep 25, 2021

bors commented Sep 25, 2021

jyn514 commented Sep 25, 2021

steffahn commented Sep 25, 2021

steffahn commented Sep 25, 2021

bors commented Sep 25, 2021

bors commented Sep 25, 2021

bors commented Sep 25, 2021

rust-timer commented Sep 26, 2021

steffahn commented Sep 26, 2021

rylev commented Oct 1, 2021 • edited Loading

steffahn commented Aug 25, 2021 •

edited

Loading

jyn514 Sep 25, 2021 •

edited

Loading

steffahn commented Sep 6, 2021 •

edited

Loading

jyn514 Sep 25, 2021 •

edited

Loading

rylev commented Oct 1, 2021 •

edited

Loading