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

Fix various bugs in match_version #565

Merged
merged 2 commits into from Jan 23, 2020
Merged

Conversation

@jyn514
Copy link
Member

jyn514 commented Jan 14, 2020

  • Ignore yanked versions
  • Ignore pre-release versions
  • Add lots of tests

Note: this now queries releases.version instead of crates.versions
so that I have accessed to releases.yanked. This shouldn't affect
behavior, but if a previous bug set crates.versions to something
different, it will no longer be seen.

Closes #223, closes #221

r? @pietroalbini

- Ignore yanked versions
- Ignore pre-release versions
- Add lots of tests

Note: this now queries `releases.version` instead of `crates.versions`
so that I have accessed to `releases.yanked`. This _shouldn't_ affect
behavior, but if a previous bug set `crates.versions` to something
different, it will no longer be seen.

Closes #223, #221
@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Jan 17, 2020

Could someone else review this?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 18, 2020

Looks good to me but as not being familiar with this code, I think a third reviewer point of view would be appreciated. :)

@jyn514

This comment has been minimized.

Copy link
Member Author

jyn514 commented Jan 22, 2020

@koenaad would you be interested in reviewing this PR?

@kvrhdn

This comment has been minimized.

Copy link
Contributor

kvrhdn commented Jan 22, 2020

@jyn514 yeah sure, I'll take a look at it later today. I don't feel super confident in my rust knowledge, but I should be able to review this 🙂

@jyn514

This comment has been minimized.

Copy link
Member Author

jyn514 commented Jan 22, 2020

Awesome, thank you! It shouldn't be super complicated, feel free to ask here or on Discord if you have any questions.

@kvrhdn
kvrhdn approved these changes Jan 22, 2020
@kvrhdn

This comment has been minimized.

Copy link
Contributor

kvrhdn commented Jan 23, 2020

These changes look good to me, good work :)

I don't have any comments on the code itself, just some general notes (not blocking for this PR):

  • While the test case looks complete and it seems to cover all edge cases. It's a bit long and thus difficult to read (in my opinion). It might be nice to split up the test into smaller tests that each cover a specific functionality, something like test_prereleases_match_with_any, test_yanked_crates_are_not_matched,... Ideally just one test should fail if some of the code changes, quickly indicating what functionality broke. It also documents how it is supposed to work.
    But in the end it's mostly personal preference, so pick whatever feels most intuitive for you while developing 😄

  • If I look at where match_version is used, it's always followed by a query on the database fetching the rustdoc_status.

    Maybe we can change match_version to expose some additional information that makes the next queries easier or, even better, unnecessary?
    A simple tweak would be to return the id of the release, making the next query easier.

@jyn514

This comment has been minimized.

Copy link
Member Author

jyn514 commented Jan 23, 2020

It might be nice to split up the test into smaller tests that each cover a specific functionality

Good call, done.

If I look at where match_version is used, it's always followed by a query on the database fetching the rustdoc_status.

Hmm, I see the comment saying 'this should be part of match_version' but I'm not sure why - giving back the latest version with docs is very different from the latest version, that would be a big change in behavior. One of the things I was explicitly asked for #516 was to make sure it did not redirect to the latest successful version so people knew a later version was published.

Returning the id sounds like a good refactor, but I think that should go in a different PR, I'll open an issue.

@kvrhdn

This comment has been minimized.

Copy link
Contributor

kvrhdn commented Jan 23, 2020

Hmm, I see the comment saying 'this should be part of match_version' but I'm not sure why - giving back the latest version with docs is very different from the latest version, that would be a big change in behavior.

Yeah, I agree it does not make much sense anymore given the current context... changing the implementation would break the usage of match_version in other places.

Returning the id sounds like a good refactor, but I think that should go in a different PR, I'll open an issue.

Agree 👍

@jyn514 jyn514 merged commit 9c0d43f into rust-lang:master Jan 23, 2020
2 checks passed
2 checks passed
Test
Details
Docker
Details
@jyn514 jyn514 deleted the jyn514:versionless-url branch Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.