Skip to content
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

Add json_metadata property to BaseDistribution #11095

Merged
merged 3 commits into from Jun 24, 2022

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented May 7, 2022

As suggested by @pfmoore in #10771 (comment), this copies the msg_to_json function from his pkg_metadata project.

This function will be used in pip install --report (and possibly pip list --format=json).

Towards #53 and #10771.

@sbidoul sbidoul added the skip news Does not need a NEWS file entry (eg: trivial changes) label May 7, 2022
@pfmoore
Copy link
Member

pfmoore commented May 7, 2022

How important is it that msg_to_json takes a Message argument? I'm considering changing it in pkg_metadata to take a bytes object (and hence hiding the dependency on email.message) as that makes the normal usage (read a metadata file) marginally simpler. But if conversion from Message format is useful, I'll leave that available as well.

@sbidoul
Copy link
Member Author

sbidoul commented May 7, 2022

@pfmoore In this case, it was convenient to have it since BaseDistribution exposes metadata as an email.message.Message. If pkg_metada would not have had that capability I'd have needed to refactor a bit.

So from this unique sample, I say this is useful. In other places where I've had to handle package metadata, email.message.Message is also involved, so it is certainly a convenient intermediate representation.

That said, exposing functions that handle metadata file encodings properly in pkg_metadata would certainly be useful too as I remember it is a topic that is not obvious in the PEP texts.

@pfmoore
Copy link
Member

pfmoore commented May 7, 2022

Sounds good. I can expose msg_to_json and a thin wrapper for bytes_to_json easily enough.

return field.lower().replace("-", "_")


def msg_to_json(msg: Message) -> Dict[str, Any]:
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be msg_to_dict instead since the output is not actually JSON (which should be a str), but a dict decoded from JSON metadata.

]


def json_name(field: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Let’s make this private so we don’t need to worry about json_name being much too vague…

Copy link
Member

Choose a reason for hiding this comment

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

It's not in __all__ in the original implementation, but agreed a better name might be good.

Would you care to review https://github.com/pfmoore/pkg_metadata? I'd be happy to incorporate any other feedback you might have in the original library.

@@ -359,6 +363,17 @@ def metadata(self) -> email.message.Message:
"""
raise NotImplementedError()

@property
def json_metadata(self) -> Dict[str, Any]:
Copy link
Member

Choose a reason for hiding this comment

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

Same for this property name; json feels misleading 😟

@sbidoul
Copy link
Member Author

sbidoul commented May 27, 2022

Hi @uranusjr, thanks for the review!

Actually this json conversion function is copied verbatim from @pfmoore 's pkg_metadata, as the goal is to vendor it when Paul says it is good enough for that. That is also the reason why there are no tests for it here. See also comment

The BaseDistribution.json_metadata property is the way I chose to expose it to pip internals and the rest is an implementation detail that will change as soon as we vendor pkg_metadata. What I could do did is rename json.py to _json.py to make it more explicit that it is "private".

I felt a property was appropriate since there was already a metadata property returning a Message.

I agree json_metadata is slightly misleading as the value is a dict and not a json string or bytes. But TBH I have no better idea that would also express the fact that this dictionary is json-serializable according to the transformation specified in PEP 566. Perhaps json_metadata_dict would be better ?

@sbidoul
Copy link
Member Author

sbidoul commented Jun 6, 2022

@uranusjr @pfmoore how do you think we should move forward with this one ?

@pfmoore
Copy link
Member

pfmoore commented Jun 6, 2022

I’ll probably change the name to msg_to_dict in pkg_metadata. I don’t really have a strong opinion on the name to use here. I’m fine with keeping json_metadata, but I’d also be ok with something like metadata_dict. Whatever we choose, we can easily change it later, after all… 🤷

@sbidoul sbidoul requested a review from uranusjr June 20, 2022 22:08
@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Jun 23, 2022
To make it explicit that it is a private implementation detail.
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jun 23, 2022
:raises NoneMetadataError: If the metadata file is available, but does
not contain valid metadata.
"""
return msg_to_json(self.metadata)
Copy link
Member

Choose a reason for hiding this comment

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

This could be cached, too. There's no real reason to recalculate it every time. Although it's probably not a performance bottleneck, so it's not a big deal either way.

@sbidoul
Copy link
Member Author

sbidoul commented Jun 24, 2022

@uranusjr I think all your concerns have been addressed. If not please do not hesitate to say so and I'll be happy to address them in a followup.

Thanks for the review and thanks @pfmoore for pkg_metadata. When you think it is ready for prime time, ping me and I'll do the vendoring.

@sbidoul sbidoul merged commit 4ecf68e into pypa:main Jun 24, 2022
@sbidoul sbidoul deleted the json-metadata-sbi branch June 24, 2022 09:15
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants