-
Notifications
You must be signed in to change notification settings - Fork 968
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
Metadata 2.2 and 2.3 #13606
Metadata 2.2 and 2.3 #13606
Conversation
56ad671
to
8197978
Compare
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.
The only thing I would want to see to call this shippable is a test that exercises the storage/retrieval of Dynamic fields.
I think this could be added to tests.unit.forklift.test_legacy:test_successful_upload
or maybe more favorably built out as an additional test tests.unit.forklift.test_legacy:test_successful_upload_metadata_2_3
.
Just some assertions of the resulting value of Release.dynamic
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.
Ported outstanding comments from #11380
Missed that Provides-Extra also needs to be stored. Similarly a test for storage/retrieval of that data would be great to see. |
We probably want to expose this value in the existing JSON API, XMLRPC API, and.. .maybe in the UI? Not sure. |
Hi @konstin, wanted to check in and see if you planned to continue work on this. If not I'm happy to pick it up and continue on with this PR. |
@dstufft, Which value, Provides-Extra? |
|
I will implement this in JSON API, but won't touch XMLRPC. There's nowhere we would expose this on the XMLRPC that isn't currently slated for deprecation. If people want programatic access to this data, they should retrieve it from JSON. I'll give a smol attempt to render it in the web UI, but consider it beyond scope of this PR. |
@di @dstufft @miketheman, this is good for a review pass. |
Sorry for responding so slow and thank you for addressing all the review comments! |
Should i rebase this? |
@konstin Yes please! |
a45b5d5
to
cf2c8d0
Compare
rebased and updated with the changes from main |
b66a10f
to
bd62fa6
Compare
Is there a way to tell the linters and formatters (or the |
I tried to address all review comments, i had to change some of the suggested changes though to make it work |
Tough to say exactly, since I don't use the same .venv style, as most development happens inside a container. You might want to add a .venv directory entry to https://github.com/pypi/warehouse/blob/main/.dockerignore so that it's not added to the container - give that a shot and if it works, feel free to add that to its own PR. |
bbc4cbb
to
c33e264
Compare
@konstin FYI, this PR has linting/testing errors (my most recent suggestion above was missing imports). |
Co-authored-by: Dustin Ingram <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]>
81ed82c
to
70e2a39
Compare
Thanks, i totally missed that CI failure! |
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.
LGTM, is blocked on merging #15452 first.
Leaving this unmerged a bit longer to give other @pypi/warehouse-committers a chance to review. |
🎉 Thanks for the reviews and guidance! |
This is an attempt to finish #11380 and unblock uploading metadata version 2.2 and 2.3, which i'd like to have for maturin and my resolver prototype. I've applied all review comments and added a check that dynamic isn't used <2.2.
Fixes #9660, fixes #11526, closes #11380.