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.

Sign up for GitHub

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

Jump to bottom

rustdoc: Link to the docs on namespaces when an unknown disambiguator is found #83866

Merged
merged 2 commits into from
Apr 8, 2021
Merged

rustdoc: Link to the docs on namespaces when an unknown disambiguator is found #83866

merged 2 commits into from
Apr 8, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Apr 5, 2021

cc #83859

@lopopolo does this look about like what you expected?

r? @camelid

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-diagnostics Area: Messages for errors, warnings, and lints A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name labels Apr 5, 2021
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2021
lopopolo
lopopolo approved these changes Apr 5, 2021
View reviewed changes
Copy link
Contributor

@lopopolo lopopolo left a comment

Choose a reason for hiding this comment

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

This looks great thank you.

camelid
camelid reviewed Apr 6, 2021
View reviewed changes
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

Could be nice to add a test to make sure the link versioning is done correctly, but it's probably not worth it. Two nits, r=me after that.

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved
src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved
@camelid
Copy link
Member

camelid commented Apr 6, 2021 •
edited

FYI, you have merge conflicts.

@camelid camelid 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 Apr 6, 2021
@jyn514
Copy link
Member Author

jyn514 commented Apr 6, 2021

Could be nice to add a test to make sure the link versioning is done correctly, but it's probably not worth it. Two nits, r=me after that.

This isn't possible, there's no way to run a nightly compiler as if it's stable (cc #83722)

@bors

This comment has been minimized.

Sign in to view
@jyn514 jyn514 force-pushed the disambiguator-error branch 2 times, most recently from cb8fea9 to 8fda6df Compare April 6, 2021 19:01
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514
Copy link
Member Author

jyn514 commented Apr 6, 2021

Two nits, r=me after that.

@bors r=camelid

@bors
Copy link
Contributor

bors commented Apr 6, 2021

📌 Commit 4f77f9b has been approved by camelid

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 6, 2021
camelid
camelid reviewed Apr 6, 2021
View reviewed changes
src/librustdoc/clean/types.rs Outdated
Comment on lines 886 to 889
// NOTE: intentionally doesn't pass crate name to avoid having
// different primitive links between crates
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming you removed this comment because it's outdated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes: that was a comment about the UnstableFeatures call, which was removed.

src/librustdoc/lib.rs Show resolved Hide resolved
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 7, 2021
@Dylan-DPC
Rollup merge of rust-lang#83866 - jyn514:disambiguator-error, r=camelid
59a2d66 
rustdoc: Link to the docs on namespaces when an unknown disambiguator is found

cc rust-lang#83859

`@lopopolo` does this look about like what you expected?

r? `@camelid`
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Apr 7, 2021
Closed
@bors

This comment has been minimized.

Sign in to view
@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 7, 2021
@bors

This comment has been minimized.

Sign in to view
@jyn514
Copy link
Member Author

jyn514 commented Apr 8, 2021

@bors r=camelid

@bors
Copy link
Contributor

bors commented Apr 8, 2021

📌 Commit 3478f83 has been approved by camelid

@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 Apr 8, 2021
@camelid
Copy link
Member

camelid commented Apr 8, 2021

I notice that there a few places in main.js that hardcode links to the stable docs. Would you like to open a followup to use the new function there as well?

@jyn514
Copy link
Member Author

jyn514 commented Apr 8, 2021

@camelid that isn't possible, because main.js is a static file and can't be modified at runtime (it's in the commit message).

@camelid
Copy link
Member

camelid commented Apr 8, 2021

@camelid that isn't possible, because main.js is a static file and can't be modified at runtime (it's in the commit message).

Yes, it's a static file, but we know what channel we're on when we include the static file, right?

@camelid
Copy link
Member

camelid commented Apr 8, 2021

Modifying the file directly would probably be annoying and would require weird template magic, but we could generate a static file that contains the toolchain info, and main.js could load that.

@jyn514
Copy link
Member Author

jyn514 commented Apr 8, 2021

Yes, it's a static file, but we know what channel we're on when we include the static file, right?

I don't know what you mean by "when we include the static file". That happens at compile-time with include_str!; rust_lang_org_channel is computed at runtime because it has a match. I could make it a const fn, but it still can't be used from within a macro.

@jyn514
Copy link
Member Author

jyn514 commented Apr 8, 2021

we could generate a static file that contains the toolchain info, and main.js could load that.

That seems like overkill for just linking to /nightly/ instead of /stable/ though?

@camelid
Copy link
Member

camelid commented Apr 8, 2021

Yes, it's a static file, but we know what channel we're on when we include the static file, right?

I don't know what you mean by "when we include the static file". That happens at compile-time with include_str!; rust_lang_org_channel is computed at runtime because it has a match. I could make it a const fn, but it still can't be used from within a macro.

Oops, I see what you mean now. I got confused by the difference between "compiling rustdoc", "running rustdoc", and "running rustdoc JS". It's like bootstrapping ;)

we could generate a static file that contains the toolchain info, and main.js could load that.

That seems like overkill for just linking to /nightly/ instead of /stable/ though?

Yeah, I guess it's not worth it after all 🙃

@bors
Copy link
Contributor

bors commented Apr 8, 2021

⌛ Testing commit 3478f83 with merge 1408715...

@bors
Copy link
Contributor

bors commented Apr 8, 2021

☀️ Test successful - checks-actions
Approved by: camelid
Pushing 1408715 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 8, 2021
@bors bors merged commit 1408715 into rust-lang:master Apr 8, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 8, 2021
This was referenced Apr 8, 2021
Closed
Merged
Closed
@jyn514 jyn514 deleted the disambiguator-error branch April 8, 2021 10:35
@pietroalbini pietroalbini mentioned this pull request May 5, 2021
Merged
@pietroalbini
Copy link
Member

pietroalbini commented May 5, 2021 •
edited

Note that this PR is being fully reverted by #84950.

@jyn514
Copy link
Member Author

jyn514 commented May 5, 2021

@pietroalbini please don't, I left a comment on that PR that should fix the beta failure.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 6, 2021
@Dylan-DPC
Rollup merge of rust-lang#84950 - pietroalbini:revert-3478f83c0, r=Ma…
ab65989 
…rk-Simulacrum

Revert PR 83866

rust-lang#83866 caused multiple test failures when Rust's channel is changed from nightly to anything else. The PR will have to be landed again after the test suite is fixed.

The two kinds of test failures were:

* Rustdoc tests failed because the links in the generated HTML didn't point at nightly anymore, see rust-lang#84909 (comment)
* Rustdoc UI tests failed because the links included in error messages didn't point at nightly anymore, see rust-lang#84909 (comment)

r? `@Mark-Simulacrum`
cc `@jyn514`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 6, 2021
@Dylan-DPC
Rollup merge of rust-lang#84950 - pietroalbini:revert-3478f83c0, r=Ma…
ccc820e 
…rk-Simulacrum

Revert PR 83866

rust-lang#83866 caused multiple test failures when Rust's channel is changed from nightly to anything else. The PR will have to be landed again after the test suite is fixed.

The two kinds of test failures were:

* Rustdoc tests failed because the links in the generated HTML didn't point at nightly anymore, see rust-lang#84909 (comment)
* Rustdoc UI tests failed because the links included in error messages didn't point at nightly anymore, see rust-lang#84909 (comment)

r? ``@Mark-Simulacrum``
cc ``@jyn514``
@jyn514 jyn514 mentioned this pull request May 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camelid camelid camelid left review comments

@lopopolo lopopolo lopopolo approved these changes

Assignees

@camelid camelid

Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.53.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@jyn514 @rust-highfive @camelid @bors @rust-log-analyzer @pietroalbini @lopopolo @rustbot