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

Convert _all_ the intra-doc links #1022

Merged
merged 2 commits into from
Nov 2, 2020
Merged

Conversation

jyn514
Copy link
Contributor

@jyn514 jyn514 commented Oct 7, 2020

Motivation

Closes #940.

Solution

Uses a heavily patched version the latest master version of https://github.com/poliorcetics/cargo-intraconv/ to automatically convert the links. Fixes remaining issues by hand.

@jyn514 jyn514 requested review from hawkw, jtescher, yaahc and a team as code owners October 7, 2020 02:25
@hawkw
Copy link
Member

hawkw commented Oct 7, 2020

It looks like there are a couple link resolution errors failing the CI build: https://app.netlify.com/sites/tracing-rs/deploys/5f7de8e1a5645300075f7ef5

@jyn514
Copy link
Contributor Author

jyn514 commented Oct 7, 2020

Right, those are poliorcetics/cargo-intraconv#13. I'm hoping to fix it upstream so the fix gets more testing, but if it takes more than a week or so I'll fix the issue manually.

@poliorcetics
Copy link
Contributor

I just pushed a fix with tests so hopefully you can use it now without having to check most links manually.

@jyn514
Copy link
Contributor Author

jyn514 commented Oct 13, 2020

This is finally ready to go! I recommend reading the second commit message - in particular, I couldn't find SubscriberBuilder::with_filter anywhere.

Giant thank you to @poliorcetics for making this possible ❤️

Copy link
Collaborator

@yaahc yaahc 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 so nice

@jyn514 jyn514 changed the title [WIP] Convert _all_ the intra-doc links Convert _all_ the intra-doc links Oct 14, 2020
@davidbarsky
Copy link
Member

Thanks for doing this! I'd like to ask that we hold off on merging this until #1015 lands because I'm nervous about the rebases already!

@davidbarsky
Copy link
Member

Good news: #1015 landed!
Bad news: There's a lot of merge conflicts now.
Less bad news: this might not be too painful to resolve.

@hawkw
Copy link
Member

hawkw commented Oct 28, 2020

It looks like this still needs to be rebased?

@jyn514
Copy link
Contributor Author

jyn514 commented Oct 28, 2020

Yes, rebasing turned out to be a pain (unsurprisingly). Working on it now, thanks for the ping.

@jyn514
Copy link
Contributor Author

jyn514 commented Oct 28, 2020

Should be fixed now.

A meta note: the terminology is really confusing. There's Subscribe which has a main fmt::Subscriber, sure, but also a reload::Subscriber and Subscriber in tracing-journald? And you also refer to the trait Subscribe as Subscriber in several places, and subscribe as 'composing' subscribers, and trait Collect as 'collectors'. I think I figured it all out, but I'm not 100% sure.

@jyn514
Copy link
Contributor Author

jyn514 commented Oct 29, 2020

There are still some more links that could be converted (mostly docs.rs or doc.rust-lang.org) but this has enough changes I think I'll save them for a follow-up PR.

@hawkw
Copy link
Member

hawkw commented Oct 29, 2020

Should be fixed now.

A meta note: the terminology is really confusing. There's Subscribe which has a main fmt::Subscriber, sure, but also a reload::Subscriber and Subscriber in tracing-journald? And you also refer to the trait Subscribe as Subscriber in several places, and subscribe as 'composing' subscribers, and trait Collect as 'collectors'. I think I figured it all out, but I'm not 100% sure.

cc @davidbarsky

@davidbarsky
Copy link
Member

A meta note: the terminology is really confusing. There's Subscribe which has a main fmt::Subscriber, sure, but also a reload::Subscriber and Subscriber in tracing-journald? And you also refer to the trait Subscribe as Subscriber in several places, and subscribe as 'composing' subscribers, and trait Collect as 'collectors'. I think I figured it all out, but I'm not 100% sure.

@jyn514 Sorry about that. I'd like to resolve consistencies such as referring to the trait Subscribe as a Subscriber (formatting choice intentional) and the trait Collect as Collector before making a proper release. Here's the styleguide that we're considering:

  • Collect and Subscribe, with the code formatting, refer to the traits.
  • Collector and subscriber, with no code formatting or special casing, refer to types implementing the Collect and Subscribe traits.
  • Types implementing Collect and Subscribe should be suffixed with Collector and Subscriber, respectively.

I was thinking of adding a glossary in a prominent location in documentation that we can point folks to. What do you think?

@jyn514
Copy link
Contributor Author

jyn514 commented Oct 29, 2020

A glossary sounds really nice :)

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

thank you so much for working on this, i know it was probably a bit of a slog! <3

@jyn514
Copy link
Contributor Author

jyn514 commented Nov 2, 2020

I have to test out rustdoc somehow! 😆

@hawkw hawkw merged commit 879b350 into tokio-rs:master Nov 2, 2020
@jyn514 jyn514 deleted the more-intra-doc-links branch November 2, 2020 17:52
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.

docs: use intra-RustDoc links rather than URLs
5 participants