pypa / flit

Simplified packaging of Python modules
https://flit.pypa.io/
BSD 3-Clause "New" or "Revised" License
2.16k stars 134 forks source link

Consider making it possible to fail on unknown keys in `[project]` table #505

Open Carreau opened 2 years ago

Carreau commented 2 years ago

I just scratched my head for 30 minutes because of a typo.

Do @takluyver think it could be ok to warn on extra keys ?

takluyver commented 2 years ago

I think warning makes sense at least. I think I tried to start a discussion when the PEP was being discussed about whether backends should actually error on unexpected keys, but it didn't really get anywhere.

pradyunsg commented 2 years ago

If metadata is improperly specified then tools MUST raise an error to notify the user about their mistake.

I guess... it depends on what "improperly specified" means? :P

None the less, we should definitely add a warning for these things.

Carreau commented 2 years ago

Ok, it seem that my installation of lit was just borked as it should already do it...

Not sure what is/was wrong.

pradyunsg commented 2 years ago

https://github.com/pypa/flit/blob/c479f6637b06907128a0c202690ef8ff7f756a8f/flit_core/flit_core/config.py#L410-L411

What @Carreau is referring to, I believe.

Carreau commented 2 years ago

Yeah, and I've been scratching my head with a project that had requires = instead of dependencies =, and trying to figure out what it would fail building...

So let's change this request for maybe:

would it be ok to have a

[tools.flit]
fail_on_unknown_pep621_keys = True

turn the warning into error ?

takluyver commented 2 years ago

The 'improperly specified' thing almost seems tautological. OK, we'll raise an error if it's wrong, but is an extra key wrong? Might the spec be extended in the future? And if so, will the extensions be optional extras which you can ignore without it really mattering, or will we need them to be handled?

Honestly, I'd be happy enough to decide the warning was a mistake and it should just be an error, without adding a new option. If/when new fields are added, Flit should add support for them and people should depend on the relevant minimum version of flit_core. Technically, this could break things if people have already published packages with incorrect keys under [project], but hopefully they haven't, and doing it soon would prevent more such broken packages being published.

Carreau commented 2 years ago

I'm tempted to make it and error as well, it's easier to make it stricter early than later.

pradyunsg commented 2 years ago

Honestly, I'd be happy enough to decide the warning was a mistake and it should just be an error, without adding a new option.

Likewise. I think it's better to error out on unknown keys here.

Might the spec be extended in the future? And if so, will the extensions be optional extras which you can ignore without it really mattering, or will we need them to be handled?

It might, and then you'll need a newer version of flit to handle them if they're specified. If they're not specified, then... well... we can indeed just consume that. :)

Overall though, I don't think there's much churn anticipated for the project table TBH, since the stuff there is basically... a 1:1 map for Core Metadata and no one is planning on fundamentally changing that in a backwards-incompatible way AFAIK. :)

takluyver commented 2 years ago

OK, who wants to do a PR? :slightly_smiling_face:

Carreau commented 2 years ago

OK, who wants to do a PR? 🙂

J'ai un draft de la faire configurable.