pypa / pyproject-metadata

PEP 621 metadata parsing
https://pep621.readthedocs.io
MIT License
31 stars 17 forks source link

Make it possible to know about "unknown" keys that might be set in `[project]` table #12

Closed pradyunsg closed 12 hours ago

pradyunsg commented 2 years ago

Inspired by https://github.com/pypa/flit/issues/505.

FFY00 commented 2 years ago

Actually, we should go one step further and try to identify typos like in https://github.com/pypa/build/blob/96f9188ad181907fbd3e0efdf32dd3dc959d39c3/src/build/__init__.py#L177.

pradyunsg commented 2 years ago

As long as the caller is able to control how these would be presented, it'd satisfy my usecases!

The use case: In https://github.com/pradyunsg/sphinx-theme-builder, all output is piped through a ui.log function for stylizing and prefixing it, and all errors/warnings are presented as an instance of a rich-renderable exception).

FFY00 commented 2 years ago

Well, I intended to raise a warning, like pypa/build. You can specify your own custom warning handler by overwriting warnings.showwarning to decide how the warnings are presented to the user (see https://github.com/pypa/build/blob/96f9188ad181907fbd3e0efdf32dd3dc959d39c3/src/build/__main__.py#L64). I think this provides all the control needed for your use-case, right?

pradyunsg commented 2 years ago

Sounds good to me!

pradyunsg commented 2 years ago

Thinking about this a bit more... I think it would be great if the warning object has the "unknown" keys accessible directly as an attribute.

pradyunsg commented 8 months ago

Based on a more recent usecase and "interaction with the real world", I would prefer that this be a validation error instead; since a build-backend should error out if it gets metadata here that's not declared properly.

If someone wants to allow for permissive parsing, I don't imagine they would be using this library for that.

FFY00 commented 5 months ago

The main worry there would be due to backwards compatibility in [project] table, as it would prevent projects that use new keys that we currently don't know about from building.

Looking into this a bit more, I think a reasonable compromise would be to keep it a warning, but to add a filter to promote it to error by default. This way it would result in an error under normal situations, but still allow users to use -Wdefault or PYTHONWARNINGS=default (or ignore) to override it.

What do you think?

rgommers commented 5 months ago

That sounds reasonable to me. In most libraries adding a filter by default would be frowned, but it shouldn't matter here because usages of pyproject-metadata are quite short-lived.

It seems difficult to introduce new keys in [project] and hence fairly unlikely to happen any time soon; an escape hatch can't hurt though.

henryiii commented 1 day ago

IMO, unlike build, which has to process any backend, this is mostly used by backends. So if a key isn't recognized, then it's not being handled, because we are the ones handling it. For example, on the new PEP 639 license fields: we current don't handle it, so users (scikit-build-core, meson-python, and pdm.backend) can't use it in combination with pyproject-metadata. Setting license to string throws an error. And setting license-files to anything doesn't put it into the RFC metadata; it just silently does the wrong thing!

And typos are very common. I found thousands of typos in my download of all pyproject.tomls on PyPI.

If a build backend wants to support new fields, they can pre-process the input. That way they can manually handle some new field without having it

I think just making this an error is fine. Though we could have build backends opt-in by calling a validate_tables function or similar. We could have errors for the top level and for build-system as well. Unlike project, though, any future additions there could be unrelated to project metadata, while everything in project is supposed to affect the metadata for the project.

henryiii commented 1 day ago

Also, the current state is inconsistent: arbitrary keys like project.stuff pass, but project.readme.stuff does not; existing entries can be modified too (license was just changed to allow a string), so if this was just for future safety, then why are sub tables checked specifically?