flit icon indicating copy to clipboard operation
flit copied to clipboard

Add option to fail on unknown keys.

Open Carreau opened this issue 2 years ago • 11 comments

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 #505

Carreau avatar Jan 07 '22 13:01 Carreau

I'd certainly be OK with having the strict option by default, and I think I'd go one step further and not make an option at all.

I just remembered we already have an environment variable FLIT_ALLOW_INVALID as an escape hatch for metadata that fails validation. That applies to a separate validation step which occurs after loading the config proper, but I don't know if that distinction is really important. Maybe that's close enough that we could reuse it for this, giving users a way round the problem if there is an unknown key, without needing another option? :shrug:

takluyver avatar Jan 07 '22 14:01 takluyver

Ok, let me see. I want to test if the FLIT_ALLOW_INVALID pass the pip install -e . and python -m build. Those are my main concern. As things get more and more built in isolated envs, I'm still sort-of thinking that a value that is part of the pyproject.toml sort of make sens.

Carreau avatar Jan 07 '22 15:01 Carreau

That's a good point. But I think a value in the pyproject.toml is perhaps not ideal, because that's controlled by the maintainers of the package, not by the people who might be trying to build it. Of course, the people downstream could patch pyproject.toml to use this option... but then they could also patch it to remove the extra keys.

takluyver avatar Jan 07 '22 15:01 takluyver

I agree, I just don't see a reason to set/unset it outside of pyproject.toml, cause if there are extra keys then the maintainers should be aware of them.

(I actually don't see a reason to unset failing on non-existing key at all unless there are new keys and an old versions of flit...).

Carreau avatar Jan 07 '22 17:01 Carreau

I want to test if the FLIT_ALLOW_INVALID pass the pip install -e . and python -m build.

I can confirm that it will work. :)

I use the same mechanism (environment variable) in another build backend: https://github.com/pradyunsg/sphinx-theme-builder/blob/97bd61c3a441341cd33c5af11c3264226c297b9e/src/sphinx_theme_builder/_internal/nodejs.py#L99

pradyunsg avatar Jan 08 '22 03:01 pradyunsg

Thanks @pradyunsg . :slightly_smiling_face:

I'm also OK with just erroring on unknown keys with no escape hatch - I think the PEP 621 support is new enough that we can probably get away with this, and we can always add an escape hatch if it turns out to be an issue. If we do want an escape hatch, the environment variable is my preference.

takluyver avatar Jan 08 '22 16:01 takluyver

It appears that at least one package has made it to PyPI with a misspelled key - pandas_dataframe_convert==0.2 has dependancies instead of dependencies.

This is probably a prompt to get this fixed quickly. But maybe also that we do need some kind of escape hatch to let such packages be built. :confused:

takluyver avatar Jan 10 '22 18:01 takluyver

Well at the same time if it’s misspelled it’s build will be incorrect.

On Mon, Jan 10, 2022 at 7:19 PM Thomas Kluyver @.***> wrote:

It appears that at least one package has made it to PyPI with a misspelled key - pandas_dataframe_convert==0.2 has dependancies instead of dependencies.

This is probably a prompt to get this fixed quickly. But maybe also that we do need some kind of escape hatch to let such packages be built. 😕

— Reply to this email directly, view it on GitHub https://github.com/pypa/flit/pull/506#issuecomment-1009199603, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACR5TY4624GSWTMCHCRHFDUVMPJHANCNFSM5LO2AP6A . You are receiving this because you authored the thread.Message ID: @.***>

Carreau avatar Jan 10 '22 19:01 Carreau

Technically incorrect but still largely working. Given what it is, I'd guess a lot of users have pandas installed anyway.

It was only up for a matter of hours before a new release corrected the issue, so in this case we'll probably get away with breaking the older release. But it does show that real people are hitting the issue and managing to upload packages to PyPI.

takluyver avatar Jan 11 '22 15:01 takluyver

Technically incorrect

The best kind of incorrect.

Carreau avatar Jan 11 '22 15:01 Carreau

My current thinking is that we'll make unknown [project] keys an error for Flit 4.0. I encourage all projects to specify an upper bound (currently <4) on the version of flit_core they build with, so that we can make changes like this without breaking existing packages.

takluyver avatar Oct 31 '22 15:10 takluyver