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

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

Open
Carreau opened this issue Jan 6, 2022 · 10 comments
Open

Comments

@Carreau
Copy link
Contributor

Carreau commented Jan 6, 2022

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

Carreau commented Jan 6, 2022

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

Not sure what is/was wrong.

@pradyunsg
Copy link
Member

if unexpected_keys:
log.warning("Unexpected names under [project]: %s", ', '.join(unexpected_keys))

What @Carreau is referring to, I believe.

@Carreau
Copy link
Contributor Author

Carreau commented Jan 6, 2022

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 ?

@pradyunsg pradyunsg changed the title warn on pep 621 unknown keys ? Consider making it possible to fail on unknown keys in [project] table Jan 6, 2022
@takluyver
Copy link
Member

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
Copy link
Contributor Author

Carreau commented Jan 6, 2022

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

@pradyunsg
Copy link
Member

pradyunsg commented Jan 6, 2022

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
Copy link
Member

OK, who wants to do a PR? 🙂

@Carreau
Copy link
Contributor Author

Carreau commented Jan 7, 2022

OK, who wants to do a PR? 🙂

J'ai un draft de la faire configurable.

Carreau added a commit to Carreau/flit that referenced this issue Jan 7, 2022
As a draft, config, and naming to discuss.

I'm tempted to make it strict by default, and have the option to ignore
if ever there are extra keys added.

Closes pypa#505
Carreau added a commit to Carreau/flit that referenced this issue Jan 7, 2022
As a draft, config, and naming to discuss.

I'm tempted to make it strict by default, and have the option to ignore
if ever there are extra keys added.

Closes pypa#505
@takluyver takluyver mentioned this issue Jan 28, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants