Serve /crate/latest/crate rather than redirect #1527

jsha · 2021-10-21T05:37:58Z

Part of #1438

syphar

Thank you for taking care of this topic :) It seems to happily serve the latest version :)

Some thoughts / missing things:

Do you mind adding some tests for this behavior?
the crate-page should probably also serve /latest/ instead of the redirect? (/crate/crate_name/latest/)
the main doc-redirect too? (/crate_name/ should probably redirect to /crate_name/latest/, not the specific version)

I'm not sure I'm missing a place where we should change a link to /latest/ instead of the version. @Nemo157 ? @jyn514 ?

src/web/rustdoc.rs

src/web/mod.rs

jsha · 2021-10-30T17:13:18Z

Alright, I've added test cases for the crate page redirects, and the doc redirects are covered by the existing test cases, which I updated. Ready for re-review.

jyn514 · 2021-10-30T18:58:39Z

@jsha do you mind implementing my MatchVersion::Latest suggestion #1527 (comment) ? Sorry to keep changing my mind, this should be the last time hopefully.

jsha · 2021-10-30T23:28:50Z

Alright, re-did with MatchSemver::Latest. That was less work than I had worried it would be, and you're right that it's tidier.

src/web/features.rs

src/web/mod.rs

jyn514 · 2021-10-31T01:30:16Z

src/web/mod.rs

@@ -351,7 +355,11 @@ fn match_version(
    {
        return Ok(MatchVersion {
            corrected_name,
-            version: MatchSemver::Semver((version.to_string(), *id)),
+            version: if input_version == Some("latest") {


Remember we also need to handle newest and *. Can you add a test case for those?

Suggested change

version: if input_version == Some("latest") {

version: if req_version == "*" {

I thought what we discussed above was that * and newest should continue to have their current behavior. That's why I specifically went for input_version rather than req_version (which gets set to * for all three cases).

My impression was they should redirect to latest, not to the exact version. It seems weird to have that be inconsistent.

(But uhhh yeah I did forget they should redirect, oops)

In what context are * and newest exposed? Part of why I wanted to leave these as-is is that I don't fully understand the use case for these alternate names. Are they a fully supported feature? How do people get to them?

I think supporting these by redirecting to latest will require an additional clause in each of the places we redirect, so I want to make sure this is something we want to do before going ahead with it.

Yes, they are fully supported in that we document them on https://docs.rs/about/redirections. "typing a url manually into the browser" is something I want to support.

I looked at fixing this and it complicates the code somewhat. We'd have to add a fourth MatchSemver variant, RedirectToLatest or similar. I propose to let * and newest continue to redirect to an exact version. It seems slightly weird, but it has the advantage of continuing the current behavior and not overcomplicating the code (and being less work :-D).

Hmm, ok - it doesn't seem like the worst thing to have a way to opt-in to the old behavior :)

src/web/builds.rs

src/web/rustdoc.rs

src/web/source.rs

jyn514 · 2021-11-24T15:04:05Z

Hi @jsha, do you know when you'll get a chance to look at this again? It's almost done I think, it just needs a few updates to the tests :) (#1527 (comment))

jsha · 2021-11-24T19:04:53Z

Thanks for the ping! I'll try to work on this this weekend. In addition to the tests there's also a pending request for a change in functionality (newest and * to redirect to latest instead of a specific version): #1527 (comment)

jsha · 2021-11-25T03:25:01Z

Updated and ready for re-review, pending resolution of #1527 (comment).

jyn514 · 2021-11-25T12:07:29Z

@jsha Can you also update the crate details page to point to /latest unconditionally? Right now they point to the latest version number instead, these tabs:

the "platform" and "details" dropdowns are also using version numbers:

also if you don't mind it would be nice to remove the merge commit by rebasing, but I don't feel strongly about that.

jsha · 2021-11-26T18:54:10Z

Will do. Also there's a similar issue going from the doc page, via the menu, to the crate page - if you're on /latest/, it will send you to a specific version.

To clarify: you said "crate details page to point to /latest unconditionally," but I think the right logic is "all links to versioned URLs point to /latest if the current page is /latest, or the version of the current page otherwise." Do you agree?

Regarding rebases: My general preference is to squash everything into one commit before merge, and also to not rebase or squash mid-review, so reviewers can easily see the diffs between revisions. In other words, please never hesitate to ask me to rebase, I'm happy to do it. I'm also of course happy to adapt to a "rebase after each round of feedback" approach if that works better for you.

jyn514 · 2021-11-26T19:19:12Z

To clarify: you said "crate details page to point to /latest unconditionally," but I think the right logic is "all links to versioned URLs point to /latest if the current page is /latest, or the version of the current page otherwise." Do you agree?

Oh oops, good catch, that's what I meant :)

My general preference is to squash everything into one commit before merge, and also to not rebase or squash mid-review, so reviewers can easily see the diffs between revisions.

Sounds great, thanks!

jsha · 2021-11-27T08:20:32Z

Pushed some fixes to the crate page and friends. That wound up being more involved than I expected, so please take a close look. I added some tests for those behaviors.

I also added a "Permalink" under the crate menu to link to a versioned URL.

jyn514 · 2021-11-27T13:29:21Z

I also added a "Permalink" under the crate menu to link to a versioned URL.

Ooh, that reminds me - github/gitlab have a y keyboard shortcut which changes the current URL to have the version number, could we do that here too? In a follow-up PR is fine, I know this one is getting kind of big.

The last thing I think we should change is for "go to latest version" to use /latest/ as well.

The /target-redirect/ page doesn't seem to support /latest/:

/crate/{crate_name}/latest/builds/184 also is broken.

(I haven't looked at the code yet, I'll do that in a second.)

jyn514

Code looks great :) thanks for writing so many tests!

src/web/rustdoc.rs

jsha · 2021-11-27T19:37:27Z

Ooh, that reminds me - github/gitlab have a y keyboard shortcut which changes the current URL to have the version number, could we do that here too? In a follow-up PR is fine, I know this one is getting kind of big.

Yep, let's do this in a followup PR.

The last thing I think we should change is for "go to latest version" to use /latest/ as well.

I'd like to propose also doing this in a followup PR. It's fairly isolatable and is in the general category of "start directing more traffic to the /latest/ URLs," along with updating the sitemap and changing crate.io.

I pushed some commits, but I just realize my fix for the builds page is incorrect (goes to a versioned page, but that's not actually what we want). I'll work on that some more and push again later today.

jyn514 · 2021-11-27T20:20:45Z

I pushed some commits, but I just realize my fix for the builds page is incorrect (goes to a versioned page, but that's not actually what we want). I'll work on that some more and push again later today.

Hmm, no that seems correct to me - we want to show a permalink because people often copy/paste it into a github issue.

jyn514 · 2021-11-27T20:23:48Z

This looks good to me as soon as you squash and fix the clippy lint :) thank you so much for sticking with this!

syphar reviewed Oct 26, 2021

View reviewed changes

syphar added the S-waiting-on-author Status: This PR is incomplete or needs to address review comments label Oct 26, 2021

jyn514 suggested changes Oct 26, 2021

View reviewed changes

src/web/rustdoc.rs Outdated Show resolved Hide resolved

jyn514 reviewed Oct 28, 2021

View reviewed changes

src/web/mod.rs Outdated Show resolved Hide resolved

jyn514 suggested changes Oct 31, 2021

View reviewed changes

jyn514 mentioned this pull request Nov 22, 2021

Make it possible to use docs.rs offline for pages that have been visited at least once #845

Open

jyn514 approved these changes Nov 27, 2021

View reviewed changes

src/web/rustdoc.rs Show resolved Hide resolved

Serve /crate/latest/crate rather than redirect

cea3de0

jsha force-pushed the stay-on-latest branch from c40def6 to cea3de0 Compare November 28, 2021 04:42

jyn514 merged commit e54f601 into rust-lang:master Nov 28, 2021

jsha deleted the stay-on-latest branch November 28, 2021 06:05

syphar mentioned this pull request Nov 28, 2021

change latest version ID on crate to match CrateDetails.latest_version #1546

Merged

Folyd mentioned this pull request Dec 6, 2021

Fix compatibility to docs.rs's new /crate/latest/crate router huhu/rust-search-extension#135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serve /crate/latest/crate rather than redirect #1527

Serve /crate/latest/crate rather than redirect #1527

jsha commented Oct 21, 2021

syphar left a comment

jsha commented Oct 30, 2021

jyn514 commented Oct 30, 2021

jsha commented Oct 30, 2021

jyn514 Oct 31, 2021

jsha Oct 31, 2021

jyn514 Oct 31, 2021

jsha Oct 31, 2021

jyn514 Nov 13, 2021 •

edited

Loading

jsha Nov 25, 2021

jyn514 Nov 25, 2021

jyn514 commented Nov 24, 2021

jsha commented Nov 24, 2021

jsha commented Nov 25, 2021

jyn514 commented Nov 25, 2021

jsha commented Nov 26, 2021

jyn514 commented Nov 26, 2021

jsha commented Nov 27, 2021

jyn514 commented Nov 27, 2021

jyn514 left a comment

jsha commented Nov 27, 2021

jyn514 commented Nov 27, 2021

jyn514 commented Nov 27, 2021

	version: if input_version == Some("latest") {
	version: if req_version == "*" {

Serve /crate/latest/crate rather than redirect #1527

Serve /crate/latest/crate rather than redirect #1527

Conversation

jsha commented Oct 21, 2021

syphar left a comment

Choose a reason for hiding this comment

jsha commented Oct 30, 2021

jyn514 commented Oct 30, 2021

jsha commented Oct 30, 2021

jyn514 Oct 31, 2021

Choose a reason for hiding this comment

jsha Oct 31, 2021

Choose a reason for hiding this comment

jyn514 Oct 31, 2021

Choose a reason for hiding this comment

jsha Oct 31, 2021

Choose a reason for hiding this comment

jyn514 Nov 13, 2021 • edited Loading

Choose a reason for hiding this comment

jsha Nov 25, 2021

Choose a reason for hiding this comment

jyn514 Nov 25, 2021

Choose a reason for hiding this comment

jyn514 commented Nov 24, 2021

jsha commented Nov 24, 2021

jsha commented Nov 25, 2021

jyn514 commented Nov 25, 2021

jsha commented Nov 26, 2021

jyn514 commented Nov 26, 2021

jsha commented Nov 27, 2021

jyn514 commented Nov 27, 2021

jyn514 left a comment

Choose a reason for hiding this comment

jsha commented Nov 27, 2021

jyn514 commented Nov 27, 2021

jyn514 commented Nov 27, 2021

jyn514 Nov 13, 2021 •

edited

Loading