Skip to content

Add multiple nums[] parameters support for GET /api/v1/crates/:name/versions #10314

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 2 commits into from
Jan 8, 2025

Conversation

eth3lbert
Copy link
Contributor

This allows us to return specified versions by filtering with the ids[] parameter.

@eth3lbert
Copy link
Contributor Author

We could also consider allowing ids[] to accept id or semver string (num).

Copy link
Contributor

@LawnGnome LawnGnome left a comment

Choose a reason for hiding this comment

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

The functionality LGTM. 👍

We could also consider allowing ids[] to accept id or semver string (num).

I think I'd prefer to keep those separate — while it's theoretically possible to distinguish integer IDs from semvers, it feels to me like it would be less surprising for users and future us if we didn't try to infer extra meaning from the provided values.

Which, given that, I'm not sure if ids[] is the best name for the parameter, since ID feels like it more refers to versions.id than versions.num. Naming is hard, so I don't know what's better, but maybe num[] or versions[] might be clearer?

@eth3lbert
Copy link
Contributor Author

I think I'd prefer to keep those separate — while it's theoretically possible to distinguish integer IDs from semvers, it feels to me like it would be less surprising for users and future us if we didn't try to infer extra meaning from the provided values.

fair enough!

Which, given that, I'm not sure if ids[] is the best name for the parameter, since ID feels like it more refers to versions.id than versions.num. Naming is hard, so I don't know what's better, but maybe num[] or versions[] might be clearer?

Given that we already responded with the field name num, I think num[] would be more straightforward. What do you think?

@Turbo87 Any thoughts?

@Turbo87
Copy link
Member

Turbo87 commented Jan 7, 2025

it took me a while to figure this out, but it looks like the original reason for the ids[] design was https://github.com/emberjs/data/blob/v4.12.8/packages/adapter/src/rest.ts#L709, which explicitly hardcodes ids[] in Ember Data.

the JSON:API spec (https://jsonapi.org/recommendations/#filtering) OTOH seems to recommend something like GET /crates?filter[id]=foo,bar. we are not using most of JSON:API, but we've used it as inspiration and guidance already in the past too. since we already started with ids[] we should probably continue with that pattern though. at least for this generation of our API...

I agree with Adam that ids[] should refer to whatever we put into the id field of the corresponding API response. If we implement support for filtering by multiple num values then nums[] would probably be most consistent with what we currently have (matches the field name and uses pluralization). Admittedly, I'm not a huge fan of it, but in terms of consistency it wins.

@eth3lbert eth3lbert changed the title Add multiple ids[] parameters support for GET /api/v1/crates/:id/versions Add multiple ids[] parameters support for GET /api/v1/crates/:name/versions Jan 7, 2025
@eth3lbert eth3lbert changed the title Add multiple ids[] parameters support for GET /api/v1/crates/:name/versions Add multiple nums[] parameters support for GET /api/v1/crates/:name/versions Jan 7, 2025
@eth3lbert
Copy link
Contributor Author

Rebased and feedback addressed!

Copy link
Contributor

@LawnGnome LawnGnome left a comment

Choose a reason for hiding this comment

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

With that change, no concerns here. 👍

@Turbo87 Turbo87 merged commit 3eb7339 into rust-lang:main Jan 8, 2025
8 of 9 checks passed
@eth3lbert eth3lbert deleted the versions-query-ids branch January 8, 2025 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants