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: Fix LinkReplacer link matching #108459

Merged
merged 1 commit into from
Jun 1, 2023
Merged

rustdoc: Fix LinkReplacer link matching #108459

merged 1 commit into from
Jun 1, 2023

Conversation

benediktwerner
Copy link
Contributor

@benediktwerner benediktwerner commented Feb 25, 2023 •
edited
Loading

It currently just uses the first link with the same href which might not necessarily be the matching one.

This fixes replacements when there are several links to the same item but with different text (e.g. [X] and [struct@X]). It also fixes replacements in summaries since those use a links list with empty hrefs, so currently all links would always match the first link by href but then not match its text. This could also lead to a panic in the original_lext[1..len() - 1] part when the first link only has a single character, which is why the new code uses .get(..) instead.

@rustbot
Copy link
Collaborator

rustbot commented Feb 25, 2023

r? @jsha

(rustbot has picked a reviewer for you, use r? to override)

@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 25, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@benediktwerner
Copy link
Contributor Author

Hm, the failing test confused me a bit but I guess it's just wrong or written with this bug in mind? Swapping the two char links on MyString2 makes the prim@char show up as char (like it will after this PR even with the current order in the test). I adjusted it.

I guess I'll also add a new test to explicitly check for the stuff this PR fixes.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member

Sorry for taking so long to review this fix. Don't hesitate to reroll it or ping someone else from the rustdoc team directly.

Anyway, just questions and then let's approve it. Thanks for working on this!

@GuillaumeGomez
Copy link
Member

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 26, 2023

📌 Commit 8fed6df has been approved by GuillaumeGomez

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 26, 2023

🌲 The tree is currently closed for pull requests below priority 100. 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 Mar 26, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 27, 2023
@Dylan-DPC
Rollup merge of rust-lang#108459 - benediktwerner:rustdoc-fix-link-ma…
185cab3 
…tch, r=GuillaumeGomez

rustdoc: Fix LinkReplacer link matching

It currently just uses the first link with the same href which might not necessarily be the matching one.

This fixes replacements when there are several links to the same item but with different text (e.g. `[X] and [struct@X]`). It also fixes replacements in summaries since those use a links list with empty hrefs, so currently all links would always match the first link by href but then not match its text. This could also lead to a panic in the `original_lext[1..len() - 1]` part when the first link only has a single character, which is why the new code uses `.get(..)` instead.
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 27, 2023
Closed
@Dylan-DPC
Copy link
Member

Dylan-DPC commented Mar 27, 2023 •
edited
Loading

failed in log of rollup

@bors

@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 Mar 27, 2023
@GuillaumeGomez
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 27, 2023

📌 Commit c92d3adc108ba5979b4f9cb5b469a6e5adaa8518 has been approved by GuillaumeGomez

It is now in the queue for this repository.

@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 Mar 27, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member

While we're at it, please squash your commits.

@bors r-

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 27, 2023
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 27, 2023
@JohnCSimon
Copy link
Member

@benediktwerner

ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@benediktwerner
Copy link
Contributor Author

Ah, sorry, looks like I missed the changes here. Fixed the formatting and squashed the commits.

@rustbot ready

@rustbot 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 May 30, 2023
@GuillaumeGomez
Copy link
Member

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 31, 2023

📌 Commit 9968f3c has been approved by GuillaumeGomez

It is now in the queue for this repository.

@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 May 31, 2023
@Dylan-DPC Dylan-DPC mentioned this pull request Jun 1, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2023
@bors
Auto merge of rust-lang#112164 - Dylan-DPC:rollup-93zj7jw, r=Dylan-DPC
fabf929 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#108459 (rustdoc: Fix LinkReplacer link matching)
 - rust-lang#111318 (Add a distinct `OperandValue::ZeroSized` variant for ZSTs)
 - rust-lang#111892 (rustdoc: add interaction delays for tooltip popovers)
 - rust-lang#111980 (Preserve substs in opaques recorded in typeck results)
 - rust-lang#112024 (Don't suggest break through nested items)
 - rust-lang#112128 (Don't compute inlining status of mono items in advance.)
 - rust-lang#112141 (remove reference to Into in ? operator core/std docs, fix rust-lang#111655)

Failed merges:

 - rust-lang#112071 (Group rfcs tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0baa301 into rust-lang:master Jun 1, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 1, 2023
@fmease fmease mentioned this pull request Jun 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@jsha jsha

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
Milestone
1.72.0
Development

Successfully merging this pull request may close these issues.
8 participants
@benediktwerner @rustbot @rust-log-analyzer @GuillaumeGomez @bors @Dylan-DPC @JohnCSimon @jsha