rustdoc: Add flag to use external source code pages #69167
Conversation
| through an HTTP URL!), you can use this option: | ||
|
|
||
| ```bash | ||
| $ rustdoc src/lib.rs -Z unstable-options --source-code-external-url 'https://somewhere.com' |
There was a problem hiding this comment.
Maybe it's a good idea to use an example domain for the purpose of being used in examples, like https://example.com as per https://www.iana.org/domains/reserved.
There was a problem hiding this comment.
I don't see the difference with what I wrote?
There was a problem hiding this comment.
somewhere.com could be owned by anyone, and they could put any content there. example.com on the other hand is registered by the Internet Assigned Numbers Authority, and it's safe to use in examples.
There was a problem hiding this comment.
That doesn't make sense to use it directly like this... Originally, I wanted to put http://a.a...
There was a problem hiding this comment.
I'm not sure where the misunderstanding is, but put simply, example.com is safe to use in examples, while any other random domain name is not (because anyone can grab those and host any content; example.com will always be safe). Sorry about the noise.
There was a problem hiding this comment.
I still don't really understand your point... Like I said, if you use it as is, it'll just not do anything except creating links to "somewhere.com". Which isn't what any user want. If you're still not convinced, I propose you the following: open a PR changing the domain I'm pointing to in the example. I'll merge it without problem. What do you think?
There was a problem hiding this comment.
Okay, that sounds good to me if we can settle there.
There was a problem hiding this comment.
Perfect! Then once this is merged, please open a PR and ping me on it so it can get merged quickly.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
GuillaumeGomez
force-pushed
the
source_code_external_url
branch
from
9dba772 to
368c777
Compare
GuillaumeGomez
force-pushed
the
source_code_external_url
branch
from
368c777 to
6f09ef6
Compare
|
Updated! |
There was a problem hiding this comment.
Why is this restricted to full URLs? That doesn't seem to be what #67804 was requesting.
Is this intended to be used by docs.rs? If so it doesn't look like this will work:
- rustdoc appends
.htmlto the filenames but docs.rs doesn't - docs.rs doesn't support the line number fragments at the end of the URL
- the paths rustdoc generates are relative to the crate root (typically
lib.rs) but docs.rs uses the root of the source archive - rustdoc replaces
..in paths withupwhich doesn't work on docs.rs
Overall I think docs.rs would be better off using redirects. They will have to solve the above issues as well but at least redirects will work for existing docs and links coming from outside docs.rs.
Oh that's certainly a problem, I didn't realize that when I read through the code. Can this be changed?
We can add this later and it doesn't affect anything in the meantime. We would have the same problem with redirects anyway.
I don't see why this is an issue? We can just pass
I'm not sure what you mean by this, can you expand a little more?
We will have to use redirects even if this is merged because it will only affect new crates, not crates with existing documentation. The benefit of this flag is
|
|
Oops, I missed
I don't think this will affect us one way or another since we can hardcode https://docs.rs/ at the front. It would be nice to use relative paths though since we have long term dreams (too far off to call them goals 😆) of companies/other people being able to host their own version of the site. |
Well docs.rs would have to pass something like
Say you have a crate #[path = "../bar.rs"]
pub mod bar;The source file for I've also noticed that this PR appends the crate name to the passed in URL which again doesn't match what docs.rs would need at all. For this to work cross crate docs.rs will have to not only pass the location of the current crate's source but also the location for all dependent crates in a similar way to |
kinnison
added
the
S-waiting-on-author
GuillaumeGomez
force-pushed
the
source_code_external_url
branch
from
6f09ef6 to
b5c573b
Compare
|
I updated how I handle the path for the external source code. I think it answers all issues reported? |
| @@ -1550,6 +1553,21 @@ impl Context { | |||
| } | |||
| } | |||
|
|
|||
| fn compute_path(path: String) -> String { | |||
There was a problem hiding this comment.
Wouldn't it be better to use canonicalize() or a URL library instead of writing this from scratch? This does not handle symbolic links for example.
I'm a little confused by this point - I thought source links only showed up in the documentation page for that struct? As long as clicking on the external type takes you to the right place, I don't see why there would need to be special handling for source urls. E.g. https://docs.rs/cranelift-faerie/0.59.0/cranelift_faerie/struct.FaerieProduct.html has a link to |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I was referring to cross crate inlining. For example on the page for |
Is it possible to do something like that outside of |
|
We could allow to have formattable values in the parameter for the crate. For example:
Or we could add another parameter to allow cross linking in such case (where the "{crate}" would still make sense). In any case, having cross-crates linking is a must have if we want to keep the same level of quality for documentation generation. What do you think? |
|
Can you give an example of how cross-crate source linking is used on docs.rs today (since we don't host std)? I'm still confused what this would look like for non-std crates. None of these suggestions are very appealing unfortunately, we'd have to add a great deal more redirections to make the |
|
I thought we generated [src] links for reexports but apparently we don't. Therefore, there is no need for this |
|
I think the only remaining issue for docs.rs is #69167 (comment), especially |
Elinvynia
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #73235) made this pull request unmergeable. Please resolve the merge conflicts. |
Elinvynia
added
S-waiting-on-author
crlf0710
added
S-waiting-on-author
Muirrum
added
S-waiting-on-author
|
r? @jyn514 |
|
Going through the concerns one at a time (and feel free to bring up any I missed):
I think it would be better to fix this here instead of immediately making a follow-up PR.
I still have this opinion but I'm willing to be convinced otherwise. It's secondary anyway, this needs design more than it needs implementation IMO.
This still hasn't been addressed. Maybe it's enough to use
This can be fixed later on the docs.rs end, I don't think it should block the rustdoc implementation.
As I mentioned above, docs.rs can add the crate root in
Do we know why rustdoc does this? Could we use
I don't see an easy way around this ... so I guess the implementation will have to allow passing |
Would it be a safe assumption to make that, by default, external crates will also have their sources hosted there? If someone is hosting their own source, maybe they would go through the effort to host the rest of the dependencies too, it's hard to say (maybe need to ask the people who want this feature about this). Definitely the cleanest option would be having and looking at per-crate metadata by default to see what external URL to use, and trusting the authors in that their external sources work (otherwise it's their problem), as opposed to using a command line flag. |
|
@Lonami sure, that's an assumption docs.rs can make. But rustdoc doesn't know the URL patterns docs.rs uses, and can't derive them from the crate names. |
|
For example, here's |
|
@GuillaumeGomez are you planning to follow up on this? |
|
Yes, just need to update it. |
|
triage: @GuillaumeGomez any updates? |
|
Completely forgot about it... Will update it as soon as possible. |
|
Actually, I just remembered that there was still discussion in #67804 about whether this should be added. |
camelid
changed the title
|
r? @camelid (although I kinda lean towards just closing this) |
|
This PR has been sitting for so long and has accrued enough conflicts that I think it's probably best to just close it. We can continue discussion on the issue. |
Fixes #67804
cc @jyn514
cc @ollie27
r? @kinnison