Skip to content

Use intra-doc links in str and BTreeSet #74470

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
Merged
merged 2 commits into from
Jul 18, 2020
Merged

Use intra-doc links in str and BTreeSet #74470

merged 2 commits into from
Jul 18, 2020

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Jul 18, 2020 •
edited
Loading

Partial fix for #32129 and #32130

A slight degradation in quality is that the #method.foo links would previously link to the same page on String's documentation, and now they will navigate to str. Not a big deal IMO, and we can also try to improve that.

@rust-highfive
Copy link
Contributor

r? @kennytm

(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 Jul 18, 2020
@Manishearth
Copy link
Member Author

This was previously reviewed in #74453

@bors r=jyn514

@bors
Copy link
Collaborator

bors commented Jul 18, 2020

📌 Commit 748634e has been approved by jyn514

@bors
Copy link
Collaborator

bors commented Jul 18, 2020

🌲 The tree is currently closed for pull requests below priority 5, 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 Jul 18, 2020
@jyn514
Copy link
Member

jyn514 commented Jul 18, 2020

@Manishearth why did you open a new PR? AFAIK the concerns in #74453 (comment) were never addressed.

@bors r-

cc @ehuss

@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 Jul 18, 2020
@Manishearth
Copy link
Member Author

@jyn514 I accidentally deleted some branches and then force pushed

It did not seem like those were blocking concerns. They cannot, at any rate, be fixed in this PR.

@jyn514
Copy link
Member

jyn514 commented Jul 18, 2020

To be clear, it is better to fix the broken links on the String page, but I consider it a backstep in other regards that should (eventually) be fixed.

Hmm ... Would it makes sense to use intra-doc links only for things that aren't primitives? Or is it not possible to use relative links because this module is re-exported in both std and core?

@Manishearth
Copy link
Member Author

Hmm ... Would it makes sense to use intra-doc links only for things that aren't primitives?

This wouldn't fix the original problem though. Note that it's not just primitives, it's methods of primitives, etc.

@jyn514
Copy link
Member

jyn514 commented Jul 18, 2020

Ok I misunderstood what happened - currently these links work in libcore but are broken in libstd. This PR makes it so they work in both crates, but link externally from libcore (they'll go to rust-lang.org/nightly). The second bit is unfortunate but IMO better than the status quo.

We should open an issue about the primitives, this has popped up a few different times now.

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 18, 2020

📌 Commit 748634e has been approved by jyn514

@bors
Copy link
Collaborator

bors commented Jul 18, 2020

🌲 The tree is currently closed for pull requests below priority 5, 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 18, 2020
@Manishearth
Copy link
Member Author

@jyn514 I was starting to draft an issue about the primitives, but I remembered that there may not be a solution here because primitives do not exist in docs before std. The code that makes this an external link is deliberately written to handle this case.

@jyn514
Copy link
Member

jyn514 commented Jul 18, 2020 •
edited
Loading

@Manishearth why are primitives in libstd and not libcore? That seems weird.

@Manishearth
Copy link
Member Author

(#73423 btw, cc @ehuss)

@ollie27
Copy link
Contributor

ollie27 commented Jul 18, 2020

Fixes #32129, fixes #32130

Those issues can't be closed until the exceptions in linkchecker are removed:

rust/src/tools/linkchecker/main.rs

Lines 122 to 127 in d3df851

// FIXME(#32129)
if file.ends_with("std/io/struct.IoSlice.html")
|| file.ends_with("std/string/struct.String.html")
{
return None;
}
and

rust/src/tools/linkchecker/main.rs

Lines 132 to 141 in d3df851

// FIXME(#32130)
if file.ends_with("alloc/collections/btree_map/struct.BTreeMap.html")
|| file.ends_with("alloc/collections/btree_set/struct.BTreeSet.html")
|| file.ends_with("std/collections/btree_map/struct.BTreeMap.html")
|| file.ends_with("std/collections/btree_set/struct.BTreeSet.html")
|| file.ends_with("std/collections/hash_map/struct.HashMap.html")
|| file.ends_with("std/collections/hash_set/struct.HashSet.html")
{
return None;
}

@jyn514
Copy link
Member

jyn514 commented Jul 18, 2020

@bors r-

@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 Jul 18, 2020
@Manishearth
Copy link
Member Author

@ollie27 hmm. Unfortunately not all of those exceptions can be removed, because some of the links are forward references, which is out of scope for intra doc links

@Manishearth
Copy link
Member Author

@ollie27 would you be okay if we can land this, fixing most of the cases, and unregistering the concern on intra doc stabilization?

@Manishearth
Copy link
Member Author

@bors r=jyn514

removed the "fixes foo" keyword from the PR body

@bors
Copy link
Collaborator

bors commented Jul 18, 2020

📌 Commit 748634e has been approved by jyn514

@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 Jul 18, 2020
@jyn514 jyn514 mentioned this pull request Jul 18, 2020
Merged
@bors bors merged commit 0d669a9 into rust-lang:master Jul 18, 2020
This was referenced Aug 3, 2020
Closed
Closed
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@kennytm kennytm

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.47.0
Development

Successfully merging this pull request may close these issues.
7 participants
@Manishearth @rust-highfive @bors @jyn514 @ollie27 @cuviper @kennytm