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

Merge this library into packaging? #103

Open
chrysle opened this issue Apr 14, 2024 · 14 comments
Open

Merge this library into packaging? #103

chrysle opened this issue Apr 14, 2024 · 14 comments
Labels
question Further information is requested

Comments

@chrysle
Copy link

chrysle commented Apr 14, 2024

This was already proposed pypa/packaging#647, but I thought I'd also file this here for greater visibility. Merging this library into
a separate module or package within packaging (since the code isn't very complex, right now) would ensure a wider availability – especially since many projects already have dependencies on packaging. That might also ease the development process. It should be a valuable addition to the current packaging's toolset.

@FFY00 FFY00 added the question Further information is requested label Apr 16, 2024
@FFY00
Copy link
Member

FFY00 commented Apr 16, 2024

I think this is mainly up to the packaging maintainers. I would be happy to collaborate and discuss this further if it is something they'd actually be interested in doing.

@henryiii
Copy link
Collaborator

@FFY00, you are the copyright owner for most of the code, is it okay to relicense the version we contribute to packaging to the packaging license? (Apache and BSD dual license)

@FFY00
Copy link
Member

FFY00 commented Oct 23, 2024

Yeah, that's fine. I should also be able to help with the move to pypa/packaging, but this depends on the maintainers there being willing to accept the code. Do you have any plan regarding this?

@rgommers
Copy link
Collaborator

@FFY00, you are the copyright owner for most of the code, is it okay to relicense the version we contribute to packaging to the packaging license? (Apache and BSD dual license)

For the record, while @FFY00 is the copyright owner of the initial commit that kick-started this project (commit d01a2c6, "import code from trampolim"), all other code written by @FFY00 is copyright Quansight since it was created during @FFY00's employment at Quansight. So also for the record: on behalf of Quansight I give permission to relicense that code to the dual Apache/BSD license of the packaging library (more than happy to do so of course, it'll be a nice outcome).

@FFY00
Copy link
Member

FFY00 commented Oct 23, 2024

@rgommers, I forgot this, sorry!

@rgommers
Copy link
Collaborator

No worries at all @FFY00! It's unlikely we'll ever need this info, but I thought it'd be good to do things by the book:)

@henryiii
Copy link
Collaborator

Do you have any plan regarding this?

I've started this at pypa/packaging#846. I've started by just making packaging.metadata.Metadata convertible back to RFC822 or JSON. I'm planning to slowly move in parts there, using as much of the existing packaging functionality as possible, and make the changes listed in #140. It'll need to be rebased once pypa/packaging#828 is in.

all other code

Maybe some of the copyright notices need updating?

@henryiii
Copy link
Collaborator

And pypa/packaging#847 is where I'm working on the addition.

@dnicolodi
Copy link
Contributor

The merge with packaging is definitely a good idea. It would be very desirable for projects (like meson-python) that depend on pyproject-metadata and that do not vendor dependencies if a very similar API could be kept with the merge.

@henryiii
Copy link
Collaborator

So far, the there are some minor differences, but everything would be easy to adapt to. We get an opportunity to make non-backward compatible changes this once. So far, the differences are:

  • (formatting) All metadata fields are lowercase
  • (formatting) The ordering of the fields is different
  • as_rfc822() is now .metadata(...).as_rfc822(), as packaging has a Metadata class already.
  • The metadata_version and dynamic_metadata arguments have been moved to metadata(...) arguments, and metadata_version is required. Currently there's no helper to automatically find the lowest supported metadata, though that's easy to compute once you move the metadata_version to .metadata().
  • ExceptionGroups always produced, no all_errors option
  • Multiline descriptions are an error.
  • There's no warn option either.

@dnicolodi
Copy link
Contributor

  • All metadata fields are lowercase

Do you mean in the RFC822 serialization? I suppose the format is case insensitive, but why make this change?

  • as_rfc822() is now .metadata(...).as_rfc822(), as packaging has a Metadata class already.

  • The metadata_version and dynamic_metadata arguments have been moved to metadata(...) arguments, and metadata_version is required. Currently there's no helper to automatically find the lowest supported metadata, though that's easy to compute once you move the metadata_version to .metadata().

I'm not sure I grok what you mean here. I understand that the API will construct an object with a metadata() method that will return a packaging.metadata.Metadata instance, and that will have a as_rfc822() method to return an email.message.Message subclass. If this is the case, it seems a bit too contrived to me.

I would have expected a from_pyproject() class method to be added to packaging.metadata.Metadata. I would also take the opportunity to simplify and get rid of the as_rfc822() method and simply add __bytes__() and __str__() implementations to turn it into the RFC 822 serialization. Especially if the producer and consumer of the RFC 822 serialization are unified into the same package, I don't see reasons why someone would want to mess with the email.message.Message subclass directly.

@henryiii
Copy link
Collaborator

henryiii commented Oct 24, 2024

but why make this change?

Because it would have to be added back in (packaging.metadata is all lower case), and since it's case insensitive, why add capitalization?

from_pyproject() class method to be added to packaging.metadata.Metadata

I'll have to look into this. I assumed it was useful to have a class to represent the [project] table, but you might be right, maybe the only intererface could be the Metadata class.

opportunity to simplify and get rid of the as_rfc822() method and simply add __bytes__() and __str__()

No, I don't like that at all! Two reasons: one, I hope that we eventually move to JSON (or anything else, really) and that's easier if we just add an .as_json() method to go along with the as_rfc822() method. And second, it might be useful to have the Message, as you can manually add or modify fields if you need to, like if you wanted to support some of the old deprecated fields. The Message returned does already have __str__ and __bytes__.

(Though, come to think of it, the old fields might actually be supported by modifying Metadata, since it has all the fields - maybe only one reason then)

@dnicolodi
Copy link
Contributor

I'll have to look into this. I assumed it was useful to have a class to represent the [project] table, but you might be right, maybe the only intererface could be the Metadata class.

packaging.metadata has already two classes for representing metadata: there is already RawMetadata and Metadata. You plan to add ProjectMetadata (or other name). Maybe adding a third is the best way forward as having a way to represent the data close to its serialization format could be useful. However, in this case I would try to have similar ways to convert among them.

RawMetadata is returned by packaging.metadata.parse_email(). Metadata is constructed from RawMetadata with its from_raw() class method. It would follow that Metadata could be constructed from ProjectMetadata with a from_project() class method. ProjectMetadata could be returned by a packaging.metadata.parse_pyproject() function (although the parse part of the name sounds slightly wrong because the input would likely be a Python dict).

Metadata can also be constructed directly with a from_email() class method. Therefore, I think a from_pyproject() class method that constructs from the pyproject.toml dictionary (going through a ProjectMetadata object, if necessary) would make sense.

This leaves with the slightly awkward Metadata.from_project() and Metadata.from_pyproject() naming, but type annotations should help resolve the difference between the two class methods.

@rgommers
Copy link
Collaborator

Maybe some of the copyright notices need updating?

Copyright notices in general are impossible to maintain and cannot be relied on, so I don't really mind. But happy to open a PR if you prefer, to update https://github.com/pypa/pyproject-metadata/blob/main/LICENSE.

One gotcha is that if you change the first line to two lines like this:

Copyright © 2019 Filipe Laíns <[email protected]>
Copyright © 2021- Quansight and pyproject-metadata contributors

then IIRC the GitHub license detection mechanism chokes and the displayed license in the right sidebar of https://github.com/pypa/pyproject-metadata may change from MIT to Other (not sure if it hasn't been fixed in the meantime). Another alternative is to just use:

Copyright © 2019- pyproject-metadata developers

Any preference?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants