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

Removed html_root_url verification logic #27

Closed
wants to merge 1 commit into from
Closed

Removed html_root_url verification logic #27

wants to merge 1 commit into from

Conversation

stanciuadrian
Copy link

html_root_url is no longer needed: rust-lang/api-guidelines@5c457df.

@mgeisler
Copy link
Owner

@stanciuadrian Oh, that's interesting! I'm a bit surprised that the attribute isn't needed any longer because docs.rs auto-configures it?

I don't know the details, but if I generate documentation locally with

$ cargo doc --no-deps

well then i still need the html_root_url attribute to get correct links for links to types from my dependencies. So it seems to me that the attribute still has a use.

Perhaps it's weird to generate documentation with --no-deps on your local machine, but imagine that I like to publish documentation to my website. That documentation should probably not include the dependencies -- I just want to document my own crate.

@hcpl
Copy link
Contributor

hcpl commented Oct 31, 2017

@stanciuadrian I can confirm what @mgeisler says.

I have a project that depends on serde and extprim. When I do

$ cargo doc --no-deps

Links to Serialize and Deserialize traits are properly generated as links to docs.rs, whereas links to i128 become plain text with no links at all. The only explanation I can think of is because serde sets html_root_url and extprim doesn't.

@mgeisler
Copy link
Owner

mgeisler commented Nov 1, 2017

@dtolnay Please take a look at the discussion here — I am basically not convinced that crates shouldn't set html_root_url just because docs.rs can auto-configure the value.

People host documentation elsewhere, most notably on various GitHub pages. That documentation will lose the links to types in dependencies unless documentation for all dependencies is generated and uploaded too.

@lnicola
Copy link

lnicola commented Nov 1, 2017

Can this be handled by rustdoc for crates which don't have html_root_url set?

@mgeisler
Copy link
Owner

mgeisler commented Nov 1, 2017

@lnicola kind of... except for crates that are not published on crates.io and which then don't have the docs on docs.rs.

So simply defaulting to docs.rs would be wrong in general.

@lnicola
Copy link

lnicola commented Nov 1, 2017

Linking to docs.rs is the best default there is. It's still possible to override the URL for crates not published on crates.io.

So this check might still be useful for crates with their documentation hosted somewhere else, as long as the version is still present in the URL.

@dtolnay
Copy link
Contributor

dtolnay commented Nov 1, 2017

Thanks for catching this, you all are right. I reverted the removal of the guideline in rust-lang/api-guidelines@b36cc0d and added a note about the correct time to remove it in rust-lang/api-guidelines@e8ca259 -- after the thing @lnicola suggested has been implemented. That is tracked in rust-lang/rust#42301. Would one of you be interested in implementing that?

@mgeisler
Copy link
Owner

mgeisler commented Nov 1, 2017

@stanciuadrian Thanks for the PR! I really appreciate the cleanup effort :-)

I went ahead and merged #26 since I think we'll keep the html_root_url functionality around for a bit longer -- at least until rust-lang/rust#42301 is implemented. When rustdoc links to docs.rs by default, then we might still want to flag outdated docs.rs links -- but we can tell the user to simply remove the attribute then.

As @lnicola said, we could even try to handle crates that host their documentation themselves: if we can detect an outdated version number, then version-sync can be helpful for those people.

Thinking about, we could simply ignore the host name: if we find any URL where the path ends with /crate-name/version/ then we have everything we need to say good/bad. That should be compatible with crates that link to docs.rs today, and with "reasonable" URLs when self-hosting.

@hcpl
Copy link
Contributor

hcpl commented Nov 2, 2017

Thinking about, we could simply ignore the host name: if we find any URL where the path ends with /crate-name/version/ then we have everything we need to say good/bad. That should be compatible with crates that link to docs.rs today, and with "reasonable" URLs when self-hosting.

Or give users ability to control the process by adding a regex or template parameter to check_html_root_url function (and likewise to assert_html_root_url_updated macro).

@stanciuadrian
Copy link
Author

You (and probably many others) are using this crate in ways I haven't anticipated when I did the PR and, yes, you are right that people may host their crates in places different from docs.rs.

Thanks for letting me know about your use-case and the --no-deps option.

@mgeisler
Copy link
Owner

Thanks @stanciuadrian for helping to keep the code clean. I hope we'll need this PR in a few months :-)

@lnicola
Copy link

lnicola commented Nov 18, 2017

@mgeisler I've looked a bit into rust-lang/rust#42301, but I don't think rustdoc can get the crate versions, so it might be a while before it's implemented.

@mgeisler
Copy link
Owner

Thanks for the update, @lnicola. We can just leave this PR around until then.

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.

5 participants