flit icon indicating copy to clipboard operation
flit copied to clipboard

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

Open Carreau opened this issue 4 years ago • 10 comments

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

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

Carreau avatar Jan 06 '22 13:01 Carreau

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.

takluyver avatar Jan 06 '22 14:01 takluyver

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.

pradyunsg avatar Jan 06 '22 14:01 pradyunsg

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

Not sure what is/was wrong.

Carreau avatar Jan 06 '22 15:01 Carreau

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

What @Carreau is referring to, I believe.

pradyunsg avatar Jan 06 '22 15:01 pradyunsg

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 ?

Carreau avatar Jan 06 '22 15:01 Carreau

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.

takluyver avatar Jan 06 '22 17:01 takluyver

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

Carreau avatar Jan 06 '22 18:01 Carreau

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. :)

pradyunsg avatar Jan 06 '22 18:01 pradyunsg

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

takluyver avatar Jan 07 '22 13:01 takluyver

OK, who wants to do a PR? 🙂

J'ai un draft de la faire configurable.

Carreau avatar Jan 07 '22 13:01 Carreau