-
Notifications
You must be signed in to change notification settings - Fork 628
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
WIP: add paginated versions API #5302
Conversation
I guess the easiest way for now would be to retrofit it to the existing endpoint but have it only paginated when
Hmm, yeah that makes it more complicated. I think it's fine if we start with just that main endpoint and potentially migrate the others later if needed.
that is unfortunate but I'm not sure I have a better solution for that right now either. I'm wondering how efficient it would be to first only load the list of version numbers for the crate, order them in rust, paginate them and the load the details. But I guess that might be incompatible with how we want the seek-based pagination to work :-/ I guess technically we could implement a Postgres extension that allows us to order by semver inside the database, though that might be a bit complicated 😅
It will need some changes, but they probably could be done separately if we modify the API in a non-breaking way. |
There is also https://github.com/theory/pg-semver, apparently, but I don't know if that works well and is supported by Heroku Postgres. Might be worth trying out though. |
As far as I know (source: https://help.heroku.com/7KM0BSLY/how-do-i-add-a-postgres-extension-to-my-database) it's not possible to install extensions on Heroku's Postgres, you can only use the preinstalled ones |
@@ -73,7 +73,7 @@ impl PaginationOptionsBuilder { | |||
self | |||
} | |||
|
|||
pub(crate) fn gather(self, req: &mut dyn RequestExt) -> AppResult<PaginationOptions> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Turbo87 I don't know if I dared too much here, I haven't seen a real motivation for that mutable reference, and in facts the rest of the code is untouched
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like a useful change, but it would be best if such refactorings were kept out of this PR for a functional change. (see chapter 6 of https://mtlynch.io/code-review-love/ for a much better explanation :D)
☔ The latest upstream changes (presumably b0bb138) made this pull request unmergeable. Please resolve the merge conflicts. |
Something nasty happened during the rebase, probably better restart from scratch... |
a33be74
to
b6b54d2
Compare
☔ The latest upstream changes (presumably #4892) made this pull request unmergeable. Please resolve the merge conflicts. |
closing in favor of #8037 |
Made some assumptions here, feel free to comment.
Still haven't checked if it need some edits frontend-side
Resolves #5292