Skip to content

Conversation

@eth3lbert
Copy link
Contributor

@eth3lbert eth3lbert commented Oct 10, 2024

This PR will add release_tracks information to the meta field for paginated versions. This is necessary for pagination because it only loads a portion of versions per page, while release_tracks may require all sorted versions to determine their values. Therefore, we also need to calculate these on the server side.


Tasks:

  • release_tracks
    • sorted by semver
    • sorted by date
    • optional by include query param

@codecov
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 99.37695% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.79%. Comparing base (f1b6235) to head (ad747b6).
Report is 122 commits behind head on main.

Files with missing lines Patch % Lines
src/controllers/krate/versions.rs 98.96% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9624      +/-   ##
==========================================
- Coverage   88.88%   88.79%   -0.09%     
==========================================
  Files         289      288       -1     
  Lines       29130    29666     +536     
==========================================
+ Hits        25892    26342     +450     
- Misses       3238     3324      +86     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eth3lbert eth3lbert force-pushed the versions-meta branch 2 times, most recently from ad9c8e0 to 8fa191f Compare October 10, 2024 20:40
@Turbo87 Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-backend ⚙️ labels Oct 11, 2024
@Turbo87
Copy link
Member

Turbo87 commented Oct 11, 2024

I thought a bit more about first_release too. I'm not sure we necessarily need it. If we decide to keep it, I think it would be fine to add a first_version_id column to the crates table instead of computing it on demand all the time. The value won't ever change once a crate is published, so we won't run into dead rows issues in the crates table.

...

and then I thought some more and realized that with version deletion being a thing soon it would be possible to delete that first version 😅

I think I'm back on "do we actually need it" :D

@eth3lbert
Copy link
Contributor Author

I thought a bit more about first_release too. I'm not sure we necessarily need it. If we decide to keep it, I think it would be fine to add a first_version_id column to the crates table instead of computing it on demand all the time. The value won't ever change once a crate is published, so we won't run into dead rows issues in the crates table.

Totally agree! I've actually been thinking about the same thing.

and then I thought some more and realized that with version deletion being a thing soon it would be possible to delete that first version 😅

I think I'm back on "do we actually need it" :D

The first_release mentioned in this PR is simply to maintain existing functionality. If it's possible to simply not display it, then I'm leaning towards not implementing it.

And due to the above reasoning, I think I'll change this PR to focus solely on release_tracks.

@eth3lbert eth3lbert marked this pull request as ready for review October 11, 2024 10:41
@eth3lbert eth3lbert changed the title Versions meta Add release_tracks meta to pagination versions Oct 11, 2024
@eth3lbert
Copy link
Contributor Author

All feedback addressed! Thanks for the review :)

This will add `release_tracks` information to the meta field for
paginated versions. This is necessary for pagination because it only
loads a portion of versions per page, while `release_tracks` may require
all sorted versions to determine their values. Therefore, we also need
to calculate these on the server side.
This enables clients to specify the `include` query param to include
`release_tracks` meta.
@eth3lbert
Copy link
Contributor Author

@Turbo87 all feedback has been addressed, and I believe this is ready to be merged. Let me know if anything needs to be changed.

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@Turbo87 Turbo87 merged commit 52fe8a0 into rust-lang:main Oct 21, 2024
10 checks passed
@eth3lbert eth3lbert deleted the versions-meta branch October 21, 2024 14:00
@eth3lbert
Copy link
Contributor Author

Hmm, it seems this doesn't have to be restricted to only available for paginated versions since you need to specify the include parameter anyway.

I'd like to have it available for non-paginated requests as well, which could make our migration to paginated versions smoother. Any thoughts, @Turbo87 ?

@Turbo87
Copy link
Member

Turbo87 commented Dec 16, 2024

I don't see any reason against it at first glance 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants