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

Make "Go to latest version" point to /latest/ #1562

Merged
merged 4 commits into from
Dec 18, 2021

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Nov 28, 2021

Still trying to get tests to pass, but figured I'd post up what I've got so far.

templates/rustdoc/topbar.html Outdated Show resolved Hide resolved
src/web/rustdoc.rs Outdated Show resolved Hide resolved
Copy link
Member

@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.

Looks great, thanks!

@jsha
Copy link
Contributor Author

jsha commented Nov 29, 2021

Sorry, forgot to comment along with the push. I updated along the lines we discussed, but I'm still struggling with getting some of the tests to pass. I think I don't fully understand what they're doing yet. If you have a chance to take a look I'd appreciate, otherwise I can probably puzzle through it next weekend.

@jyn514
Copy link
Member

jyn514 commented Nov 29, 2021

@jsha hmm, so this seems like a larger problem - target-redirect doesn't support latest, only specific version numbers. I think it's ok to change the tests to expect latest as long as we make sure that page doesn't 404.

@jsha
Copy link
Contributor Author

jsha commented Dec 1, 2021

target-redirect does appear to support latest:

$ curl -i https://docs.rs/crate/rustls/latest/target-redirect/x86_64-unknown-linux-gnu/rustls/index.html
HTTP/2 302 
content-length: 0
location: https://docs.rs/rustls/latest/rustls/index.html

@jyn514
Copy link
Member

jyn514 commented Dec 1, 2021

Ok great, I think you can just update the tests to match the new behavior then :)

@jsha
Copy link
Contributor Author

jsha commented Dec 15, 2021

Alright, I've got almost all of them updated, except this one, which now gets a 404:

thread 'web::rustdoc::test::redirect_latest_with_all_yanked::_true' panicked at 'failed to GET /crate/dummy/latest/target-redirect/x86_64-unknown-linux-gnu/dummy/index.html: 404 Not Found', src/test/mod.rs:47:5

I don't really understand - what's the expected behavior when all versions are yanked? Do I need to add code such that target-redirect will go to a yanked version if there's no unyanked version available?

@Nemo157
Copy link
Member

Nemo157 commented Dec 15, 2021

Previously, the behaviour when all versions were yanked is that you could not load the crate using any of the semver/wildcard requests, only an exact version number would load; and if you were on an old version of the crate then it would show a link to the latest version still.

I'm not sure what the behaviour should be after this change, changing the latest version link on those pages to /latest/ would presumably not work since that's not an exact version link; but not changing it would be an annoying inconsistency. One option would be to just drop the latest version redirect in those cases, a fully yanked crate is a real edge case and you probably don't want to be updating it to the latest version (especially since you would have to manually build your Cargo.lock to do the update).

@jsha
Copy link
Contributor Author

jsha commented Dec 15, 2021

By "drop the latest version redirect in those cases," you mean don't generate the link in the header?

@Nemo157
Copy link
Member

Nemo157 commented Dec 15, 2021

Exactly.

@jsha
Copy link
Contributor Author

jsha commented Dec 18, 2021

Alright, I checked and that's actually the current behavior - no link is generated when all versions are yanked. So I pulled out the failing redirect_latest_with_all_yanked test because it was testing a behavior that doesn't happen. Should be ready to go!

@jyn514 jyn514 changed the title [WIP] Make "Go to latest version" point to /latest/ Make "Go to latest version" point to /latest/ Dec 18, 2021
@jyn514 jyn514 merged commit 824daa4 into rust-lang:master Dec 18, 2021
@jsha jsha deleted the go-to-latest branch December 18, 2021 17:30
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.

3 participants