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

Rustdoc: include crate name in links for local primitives #121490

Merged
merged 1 commit into from
Feb 24, 2024

Conversation

gurry
Copy link
Contributor

@gurry gurry commented Feb 23, 2024

Fixes #121106.

This change makes links to primitives easier to use when the path of the page where they will be embedded is not known beforehand such as when we generate impls dynamically from the register_type_impls method in main.js, which is exactly what is happening in #121106.

An example to show the effect of this change: earlier, if the current page in cx.current inside primitive_link_fragment() was std::simd::prelude::Simd the generated path would be ../../primitive.<prim>.html. Now it would be ../../../std/primitive.<prim>.html instead.

A side effect of the change is that local primitive links everywhere will now contain the crate name, even outside of the dynamic situation mentioned above. I'm not sure if there are any major downsides of that other than making the links a bit longer. Ideally I wanted to restrict this behaviour change to only the dynamic cases. We could have achieved that by passing an additional bool arg to primitive_link_fragment(), but it felt awkward to do so. Any alternative suggestions are welcome.

@rustbot
Copy link
Collaborator

rustbot commented Feb 23, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 23, 2024
@gurry
Copy link
Contributor Author

gurry commented Feb 23, 2024

cc @notriddle

@notriddle
Copy link
Contributor

Ideally I wanted to restrict this behaviour change to only the dynamic cases.

To avoid making the regular standard library docs bigger, could this be done based on when cx.current.len() == 0?

match m.primitive_locations.get(&prim) {
    Some(&def_id) if def_id.is_local() && cx.current.is_empty() => {
        write!(
            f,
            "<a class=\"primitive\" href=\"{}/primitive.{}.html{fragment}\">",
            ExternalCrate { crate_num: def_id.krate }.name(cx.tcx()),
            prim.as_sym()
        )?;
        needs_termination = true;
    }
    Some(&def_id) if def_id.is_local() => {
        write!(
            f,
            "<a class=\"primitive\" href=\"{}primitive.{}.html{fragment}\">",
            "../".repeat(cx.current.len() - 1),
            prim.as_sym()
        )?;
        needs_termination = true;
    }

@notriddle notriddle assigned notriddle and unassigned fmease Feb 23, 2024
@gurry
Copy link
Contributor Author

gurry commented Feb 24, 2024

Ideally I wanted to restrict this behaviour change to only the dynamic cases.

To avoid making the regular standard library docs bigger, could this be done based on when cx.current.len() == 0?

match m.primitive_locations.get(&prim) {
    Some(&def_id) if def_id.is_local() && cx.current.is_empty() => {
        write!(
            f,
            "<a class=\"primitive\" href=\"{}/primitive.{}.html{fragment}\">",
            ExternalCrate { crate_num: def_id.krate }.name(cx.tcx()),
            prim.as_sym()
        )?;
        needs_termination = true;
    }
    Some(&def_id) if def_id.is_local() => {
        write!(
            f,
            "<a class=\"primitive\" href=\"{}primitive.{}.html{fragment}\">",
            "../".repeat(cx.current.len() - 1),
            prim.as_sym()
        )?;
        needs_termination = true;
    }

Thanks for the suggestion. I'll make the change.

@gurry gurry force-pushed the 121106-broken-usize-link-in-docs branch from 147a678 to 4722344 Compare February 24, 2024 03:39
@gurry
Copy link
Contributor Author

gurry commented Feb 24, 2024

Thanks for the suggestion. I'll make the change.

Done

@rust-log-analyzer

This comment has been minimized.

It makes the link easier to use in cases in which
the path of the page where it will be embedded is not
known beforehand such as when we generate impls
dynamically from `register_type_impls` method in
`main.js`

Earlier for local primitives we would generate a path
that was relative to the current page depth passed in `cx.current`
. e.g if the current page was `std::simd::prelude::Simd` the
generated path would be `../../primitive.<prim>.html`  After this
change the path will first take you to the the wesite root and add
the crate name. e.g. for `std::simd::prelude::Simd` the path now
will be `../../../std/primitive.<prim>.html`
@gurry gurry force-pushed the 121106-broken-usize-link-in-docs branch from 4722344 to e0bfa5c Compare February 24, 2024 05:08
@notriddle
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 24, 2024

📌 Commit e0bfa5c has been approved by notriddle

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 24, 2024

🌲 The tree is currently closed for pull requests below priority 50. This pull request will be tested once the tree is reopened.

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 24, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 24, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#121435 (Account for RPITIT in E0310 explicit lifetime constraint suggestion)
 - rust-lang#121490 (Rustdoc: include crate name in links for local primitives)
 - rust-lang#121520 (delay cloning of iterator items)
 - rust-lang#121522 (check that simd_insert/extract indices are in-bounds)
 - rust-lang#121531 (Ignore less tests in debug builds)
 - rust-lang#121539 (compiler/rustc_target/src/spec/base/apple/tests.rs: Avoid unnecessary large move)
 - rust-lang#121542 (update stdarch)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit dbe3398 into rust-lang:master Feb 24, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 24, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 24, 2024
Rollup merge of rust-lang#121490 - gurry:121106-broken-usize-link-in-docs, r=notriddle

Rustdoc: include crate name in links for local primitives

Fixes rust-lang#121106.

This change makes links to primitives easier to use when the path of the page where they will be embedded is not known beforehand such as when we generate impls dynamically from the `register_type_impls` method in `main.js`, which is exactly what is happening in rust-lang#121106.

An example to show the effect of this change: earlier, if the current page in `cx.current` inside `primitive_link_fragment()` was `std::simd::prelude::Simd` the generated path would be `../../primitive.<prim>.html`. Now it would be `../../../std/primitive.<prim>.html` instead.

A side effect of the change is that local primitive links _everywhere_ will now contain the crate name, even outside of the dynamic situation mentioned above. I'm not sure if there are any major downsides of that other than making the links a bit longer. Ideally I wanted to restrict this behaviour change to only the dynamic cases. We could have achieved that by passing an additional bool arg to `primitive_link_fragment()`, but it felt awkward to do so. Any alternative suggestions are welcome.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 27, 2024
…06, r=GuillaumeGomez

Add test case for primitive links in alias js

Follow up rust-lang#121490

CC rust-lang#121106
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 27, 2024
…06, r=GuillaumeGomez

Add test case for primitive links in alias js

Follow up rust-lang#121490

CC rust-lang#121106
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 27, 2024
…06, r=GuillaumeGomez

Add test case for primitive links in alias js

Follow up rust-lang#121490

CC rust-lang#121106
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 28, 2024
…06, r=GuillaumeGomez

Add test case for primitive links in alias js

Follow up rust-lang#121490

CC rust-lang#121106
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 28, 2024
…06, r=GuillaumeGomez

Add test case for primitive links in alias js

Follow up rust-lang#121490

CC rust-lang#121106
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 28, 2024
Rollup merge of rust-lang#121572 - notriddle:notriddle/test-case-121106, r=GuillaumeGomez

Add test case for primitive links in alias js

Follow up rust-lang#121490

CC rust-lang#121106
@gurry gurry deleted the 121106-broken-usize-link-in-docs branch April 30, 2024 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken parameter links in some of the stdlib documentation
6 participants