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

A new broken link callback design #469

Merged
merged 5 commits into from
Aug 31, 2020

Conversation

marcusklaas
Copy link
Collaborator

@marcusklaas marcusklaas commented Aug 23, 2020

This is a WIP PR to address several outstanding issues with the current broken link callback design.

Firstly, it provides additional data (source mapping, link type) to the callback to improve diagnostics (#373) and help disambiguate links with identical references (#165). Further, this design also prevents the callback from being called twice on the same reference (#444). And lastly, the callback now returns CowStrs, so that it is possible to generate titles and urls without memory allocations, for example when they are static strings or derived from text in the source.

Feedback is greatly appreciated. Would this cover your use-cases? Is this an improvement over the old design?

cc @euclio @jyn514 @GuillaumeGomez

@marcusklaas marcusklaas marked this pull request as draft August 23, 2020 16:24
Copy link
Contributor

@jyn514 jyn514 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 like a great improvement :D It definitely fits my use case in #444. I'll let @euclio comment on the other two issues.

src/parse.rs Outdated Show resolved Hide resolved
src/parse.rs Outdated Show resolved Hide resolved
@euclio
Copy link
Contributor

euclio commented Aug 25, 2020

This looks great to me!

Copy link
Contributor

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Let me know if there's anything I can do to help with this :) I'd love to have these fixed before intra-doc links (hopefully) stabilize in 6 weeks.

src/parse.rs Outdated Show resolved Hide resolved
@marcusklaas marcusklaas marked this pull request as ready for review August 30, 2020 16:51
@marcusklaas marcusklaas changed the title Initial implementation of a potential new callback design (WIP) Initial implementation of a potential new callback design Aug 30, 2020
@marcusklaas marcusklaas changed the title Initial implementation of a potential new callback design A new broken link callback design Aug 30, 2020
examples/broken-link-callbacks.rs Outdated Show resolved Hide resolved
examples/broken-link-callbacks.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

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

Generally this looks good, suggestions for minor fixes inline. I understand this is a (minorly) breaking change and we'll want to bump semver?

examples/broken-link-callbacks.rs Outdated Show resolved Hide resolved
src/parse.rs Outdated Show resolved Hide resolved
src/parse.rs Outdated Show resolved Hide resolved
@marcusklaas
Copy link
Collaborator Author

marcusklaas commented Aug 31, 2020

Sending a big thank to @jyn514, @euclio and @raphlinus for the feedback!

Edit: And yes, this will require a semver breaking change bump.

@marcusklaas marcusklaas merged commit fe43163 into pulldown-cmark:master Aug 31, 2020
@marcusklaas marcusklaas deleted the callback-redesign branch August 31, 2020 10:20
@Michael-F-Bryan
Copy link
Contributor

This is awesome, thank you @marcusklaas!

Now I just need to update mdbook-linkcheck so we can take advantage of it and remove the previous hack workaround.

jyn514 added a commit to jyn514/rust that referenced this pull request Sep 14, 2020
Thanks to marcusklaas' hard work in pulldown-cmark/pulldown-cmark#469, this fixes a lot of rustdoc bugs!

- Get rid of unnecessary `RefCell`
- Fix duplicate warnings for broken implicit reference link
- Remove unnecessary copy of links
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 15, 2020
…Gomez

Upgrade to pulldown-cmark 0.8.0

Thanks to marcusklaas' hard work in pulldown-cmark/pulldown-cmark#469, this fixes a lot of rustdoc bugs!

- Get rid of unnecessary `RefCell`
- Fix duplicate warnings for broken implicit reference link
- Remove unnecessary copy of links

Closes rust-lang#73264, closes rust-lang#76687.
r? @euclio

I'm not sure if the switch away from `locate` fixes any open bugs - euclio mentioned some in pulldown-cmark/pulldown-cmark#165, but I didn't see any related issues open for rustdoc. Let me know if I missed one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants