Skip to content

use crates.io search to search crates #1521

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

Merged
merged 4 commits into from
Nov 13, 2021

Conversation

syphar
Copy link
Member

@syphar syphar commented Oct 16, 2021

This removes our own custom search with an API-Call to crates.io.

Most of the actual API-call is based on #1224, while I reverted some parts of it.
I added rustdoc_status to the match_version output so the search_handler looks better.

Since crates or versions returned from the crates.io search might not exist in docs.rs yet, we are

  • matching the returned crate-names with our own database
  • using the old logic to find the latest version to show in the result.
  • but we are keeping the ranking/ordering.
  • pagination is purely based on the next_page and prev_page values coming from crates.io, following what @pietroalbini said in the chat.

My thoughts on the version being shown:

IMHO using max_version or max_stable_version from crates.io is out of the picture since it might not exist in docs.rs yet.
Other release-list pages use crates.latest_version_id, which is why I was thinking of using this version, but since this would be changed behavior I left the version logic as-is.

for the future

I believe we should change the content of crates.latest_version_id to match CrateDetails.latest_release. Then a click on the release-lists or search-results always takes you to a version without a go to latest version link.

So you don't have to look it up, we are showing the semver-highest version which is not yanked and no prerelease. What is different compared to the current version showed by search is that the latest_release definition includes releases with failed builds.

But I would change this (and we can discuss details) in a separate PR.

Related issues: #708, perhaps #1009

@syphar
Copy link
Member Author

syphar commented Oct 16, 2021

( creating this as draft since I'm offline most of the time, but wanted to start the review to safe time in the end )

@syphar syphar added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Oct 16, 2021
@syphar syphar self-assigned this Oct 16, 2021
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.

It's confusing to review both the added pagination and the new search behavior at the same time - do you mind splitting them up into different PRs? Note that the pagination fixes #788.

@jyn514
Copy link
Member

jyn514 commented Oct 27, 2021

(I didn't do a very thorough review here yet.)

@syphar
Copy link
Member Author

syphar commented Oct 27, 2021

It's confusing to review both the added pagination and the new search behavior at the same time - do you mind splitting them up into different PRs? Note that the pagination fixes #788.

The pagination PR would have to be based on the api-PR,
but I'm happy to split them. Or at least limit this one to the API change, and add another one when this one is finished

@jyn514
Copy link
Member

jyn514 commented Oct 27, 2021

If you just put them in two commits in the same PR, that would be fine too :)

@syphar syphar force-pushed the use-crates-io-search branch from f9c5ffd to 9b0f8ff Compare October 27, 2021 17:38
@syphar syphar requested a review from jyn514 October 27, 2021 17:38
@syphar
Copy link
Member Author

syphar commented Oct 27, 2021

If you just put them in two commits in the same PR, that would be fine too :)

I have split this into two separate commits, other fixes I postponed until after the proper review.

I hope the changes are easier to understand now :)

@jyn514
Copy link
Member

jyn514 commented Oct 29, 2021

Other release-list pages use crates.latest_version_id, which is why I was thinking of using this version, but since this would be changed behavior I left the version logic as-is.

I think we should match the other releases. @Kixiron I don't remember why we used exactly the logic we have currently; I suspect you were just trying to match the other releases and we changed the logic in the meantime?

I believe we should change the content of crates.latest_version_id to match CrateDetails.latest_release. Then a click on the release-lists or search-results always takes you to a version without a go to latest version link.

👍 this seems good, can you open an issue for it?

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.

This seems good to me, just a small nit. It's kind of broken locally because my database doesn't have almost any of the crates on crates.io, but I don't see a good way of fixing that and still using crates.io search.

@syphar syphar marked this pull request as ready for review October 29, 2021 06:40
@syphar
Copy link
Member Author

syphar commented Oct 29, 2021

Other release-list pages use crates.latest_version_id, which is why I was thinking of using this version, but since this would be changed behavior I left the version logic as-is.

I think we should match the other releases. @Kixiron I don't remember why we used exactly the logic we have currently; I suspect you were just trying to match the other releases and we changed the logic in the meantime?

I believe we should change the content of crates.latest_version_id to match CrateDetails.latest_release. Then a click on the release-lists or search-results always takes you to a version without a go to latest version link.

👍 this seems good, can you open an issue for it?

This is probably #708

This seems good to me, just a small nit. It's kind of broken locally because my database doesn't have almost any of the crates on crates.io, but I don't see a good way of fixing that and still using crates.io search.

In theory we could have a fallback/config for local development that just does xx like '%search%'. But IMHO it's not worth the effort.

@syphar
Copy link
Member Author

syphar commented Nov 13, 2021

@jyn514 was something missing here or can we merge?

@jyn514 jyn514 merged commit a4342e4 into rust-lang:master Nov 13, 2021
@syphar syphar deleted the use-crates-io-search branch November 13, 2021 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants