Skip to content

Refactor CrateDetails: add yanked field to Release #560

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jan 11, 2020
Merged

Refactor CrateDetails: add yanked field to Release #560

merged 6 commits into from
Jan 11, 2020

Conversation

yvrhdn
Copy link

@yvrhdn yvrhdn commented Jan 10, 2020

This is another small refactor from working on #516.

Changes should be easier to check looking at them commit-by-commit.

Basically, the previous implementation had to query the database to determine the last successful build.
But most of the information needed is already present in releases. By also fetching the yanked data into Release we avoid doing an extra query.
Additionally, the yanked field will be useful later to improve latest_version(), see the issues mentioned by #559 (comment).

I will have to merge / rebase these changes on top of #559. But you can already take a look :)

@jyn514
Copy link
Member

jyn514 commented Jan 10, 2020

Merged #559, time for that rebase :) I'm looking in the meantime.

jyn514
jyn514 reviewed Jan 11, 2020
View reviewed changes
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.

I started looking at this, but in the middle I discovered that we in fact show yanked crates as if they were normal crates. With that in mind, I think we should wait to refactor yanked crates until that's fixed: #561

@yvrhdn
Copy link
Author

yvrhdn commented Jan 11, 2020

I started looking at this, but in the middle I discovered that we in fact show yanked crates as if they were normal crates. With that in mind, I think we should wait to refactor yanked crates until that's fixed: #561

This PR doesn't change the behavior of last_successful_build. I have added some tests with
247d1e3 to pin down the expected results before changing the implementation.
So I don't think #561 should block this PR.

The current implementation will not show a last_successful_build warning if the build was successful (thanks to this if case: https://github.com/rust-lang/docs.rs/pull/560/files#diff-07b7024fe121ba3b5acf483a50bf3000R239).
I can update the tests to reflect this behavior? Now there is a comment saying we don't test for yanked release, since I didn't expect this to be relevant.

@yvrhdn yvrhdn marked this pull request as ready for review January 11, 2020 00:48
@jyn514
Copy link
Member

jyn514 commented Jan 11, 2020

Hmm, I suppose as long as there's no change in behavior it doesn't hurt to go ahead. Please do update the tests.

Now there is a comment saying we don't test for yanked release, since I didn't expect this to be relevant.

I think I got yanked versions mixed up with blacklisted versions before: yanked is by the user, blacklisted is by the infra team (docs.rs or crates.io). Sorry about that.

jyn514
jyn514 approved these changes Jan 11, 2020
View reviewed changes
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.

Once you add the new test cases, this looks good to me :) It's great how easy this is after all your other refactoring work.

update CrateDetails tests to handle yanked crates correctly
d87822b 
When a crate is yanked from crates.io, its documentation might not yet
be removed from docs.rs. So it is possible to create CrateDetails of a
release that was build successfully but yanked afterwards. Since this
release was built successfully, it will not have a reference to a last
successful build. But since it was yanked, other release will not point
to this release as last successful build.
@yvrhdn
Copy link
Author

yvrhdn commented Jan 11, 2020 •
edited
Loading

I have added the asserts for yanked releases now.

The test expose that the behavior is a bit funky right now... in this test 0.0.4 is the most recent successful build, but since it is yanked other releases will point to 0.0.2 as last_successful_build... 🤔

But this seems like a fairly infrequent edge-case anyway. I assume when a crate is yanked, the library author would pretty quickly want to push an updated version.

I think I got yanked versions mixed up with blacklisted versions before: yanked is by the user, blacklisted is by the infra team (docs.rs or crates.io). Sorry about that.

No worries, it is pretty confusing 😅

@jyn514
Copy link
Member

jyn514 commented Jan 11, 2020

The test expose that the behavior is a bit funky right now... in this test 0.0.4 is the most recent successful build, but since it is yanked other releases will point to 0.0.2 as last_successful_build...

That seems like the right behavior to me. Is there something you think we should do instead?

@yvrhdn
Copy link
Author

yvrhdn commented Jan 11, 2020

No, I think it's okay for now. It feels inconsistent since we don't mention anything on the yanked release, but we'll address that with #561 :)

@jyn514 jyn514 merged commit f26cc22 into rust-lang:master Jan 11, 2020
@yvrhdn yvrhdn deleted the crate_details_yanked branch January 11, 2020 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yvrhdn @jyn514