-
Notifications
You must be signed in to change notification settings - Fork 308
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
Add custom checkers to twine check
#848
Comments
twine check
twine check
@sigmavirus24 What do you think of this initial writeup? Any edits before I publicize it (e.g. to folks who submitted |
@sigmavirus24 Just noticed the 👍 (can't decide if the absence of notifications for those are good or bad). Is there anything you'd like to add, e.g. lessons learned from flake8? |
Looks reasonable. Although, I don't know if the checkers would need custom args (if they did, we'd also be talking about a config file, I suppose). |
@webknjaz Can you give an example of a checker that would need custom args? And/or can you propose how some of |
Not really, I was just commenting on the original mentions of having to implement arg parsing which doesn't seem necessary right now. |
cc @di @ssbarnea @gaborbernat @henryiii Y'all have shown some interest in improving |
I am not very keen on having plugins for these checks as it would mean that we would need to modify each project running 'twine check --strict' in order to enable to newly added checks. I do not want to have to add them to benefit from newer checks. If we can avoid that downside the better. If we add new check only to strict, we should be fine. Regarding where these checks are implemented, i do not care much, but if they are in another project, it should be a direct dependency of twine, not an optional one. |
I think the proposal is functionally sound, but I'm not sure I see the demand for third-party plugins for Assuming pypa/packaging#147 eventually happens (I see no objections there, just and lack of people to work on it) I'm having a hard time seeing what else users would want |
I think I'll likely be a little repetitive of @di and @ssbarnea if I write a full response. I don't think there are an unlimited number of checks for metadata that need to be done. There are a set of things that are needed (either for PyPI, or for consistency), but there isn't a need for an arbitrary number of things, like flake8. And, honestly, I think pre-commit has been a better solution than flake8 - instead of providing an extension system for running checks, it's an infrastructure for running anything, so there are lots of great (and modifying) checks that are not in flake8. I would see the same thing here - if you really want to add a custom check, you can write a custom checker and then run it on your wheel files, adding it to pre-commit or CI or wherever. Extending something based on what is installed in the current environment has caused issues for pytest (flaky is registered by multiple extensions, which causes issues for pypa/build). I'm very much in favor of twine check becoming more powerful, but I don't think it needs to be come externally extensible. A good design for internal use is nice, but it doesn't seem to be that helpful to be able to add For completeness, here's a few things I can think of that would be nice to check:
For "extendable" checks, the only think I would think of would be validating md or rst markup, but that can be done in the original file with a normal checker, usually. |
So I proposed #843 and tried to introduce the plugin system because I felt like it could be useful. To be honest, I'm fine with dropping the "plugin" architecture and having all checkers live in twine.
I think it should be kept simple:
The checker would take the filename of a tarball or wheel and return a list of warnings/errors. |
Lets put all checks in twine and look into moving them only if that becomes a problem, avoiding over-engineering. At some point we may want to add a noqa mode for bypassing some of them deliberately, but again, later. The more checks we add to twine strict the better. |
I disagree these should all just get thrown into twine. Specifically, everything @CyrilRoelandteNovance listed is non-essential for uploading to PyPI. Those are things other organizations what, which is exactly the reason for making It makes no sense to build checkers for every optional thing some mega-corp might want. I also don't see how we'd build a Finally, @CyrilRoelandteNovance's idea of making every plugin figure out how to unpack a distribution, etc seems incredibly naive.
This makes total sense to me
There are other tools that do this already namely https://github.com/asottile/setup-cfg-fmt @bhrutledge I think we can close this |
I think we are talking about It would be nice of wheel had a api, it would make implementing this elsewhere easier. Setup-cfg-fmt only works for one (of soon to be three) ways to configure setuptools, not for any PEP 517 build system, and it absolutely does not check this. |
It auto-fixes |
I think I missed this distinction:
It seems like there's some disagreement about whether That said, it's not clear to me what is essential for uploading a package to PyPI, and does (or should) that include being able to |
IMO,
Essentially it needs to pass a lot of metadata validation and validation of things like the filename/extension, etc, which lives at https://github.com/pypa/warehouse/blob/bf389e4acd129697b28900c962dc9d363da3b00f/warehouse/forklift/legacy.py
I think that's probably pypa/packaging#147 and #430 (which might itself be fixed by pypa/packaging#147). Everything else seems to be a 'best practice' that PyPI doesn't actually enforce, something that hasn't been fully agreed upon as a standard, or some kind of meta-improvement. |
@di Your statement is in contradiction with |
So that we're on the same page, here's the current behavior of Without
With It sounds like most people on this discussion (all of whom aren't Twine maintainers 😉) are suggesting that "best practice" checks be added as warnings, similar to a missing |
I agree with @bhrutledge that adding extra configuration complexity should be avoided, if possible. I wonder if we could make use of python I mention this because if we use these warnings, it means that power-users could also make use of |
The current behavior is to write warnings to |
That may have been the initial idea, but the initial ideas of many things sometimes don't have sufficient experience to be correct. Look at the discussion of universal wheels. They were always intended originally to be python version independent and now have transmogrified to being python 2 and Python 3 compatible because of a choice made by a popular implementation. |
I am forcefully against "best practice" checks especially when there's no source of truth just disparate groups of the opinion that the things they want are best practices. The only sane option in this case is a huge amount of contributed checks we few would have to maintain and eventually probably make configurable or pluggable architecture to let people install their best practices plugins as they see fit. The latter is seen as wholly unappealing at this point and the former us appealing to those who won't be doing the work and repulsive to me. I think it's best to close this as "twine will ensure the minimum necessary medatata has been generated and included in an artifact for use with PyPI and only PyPI". Existing feature requests that don't fit into that should be closed. |
Breaking down @sigmavirus24's observations.
@sigmavirus24 Re: option 2, do you see that as unappealing? I still find it appealing, for its potential utility, a cleaner factoring of I don't find option 1 repulsive, but I am a -1.
Before doing that, I'd like to dig into @di's thoughts in #848 (comment), and get a sense of what it would take to fulfill the promise of ensuring the minimum necessary metadata. |
Sorry I took a while, intended to post this earlier, and I think it's been indirectly mentioned: As I see it, there are three levels of checks:
Category 1 was mentioned by @di - and that's currently still lacking, and should be the highest priority. That's clearly a job for Category 2 already does exist, but only a few checks on long-description are implemented, and of those, missing fields is actually one I'd probably categorize in category 3 if I were starting from scratch. This has limited enough scope I think it could be "warnings" in I think we all agree category 1 should be part of twine, and category 3 can be implemented elsewhere. I think the best solution for allowing others to write checks is not to have a plugin system for twine, but to have some sort of API that others can use - maybe starting with wheel eventually, or even if twine provided an I think clear invalid data should be warning - this is very much category 2. For example, if If we call missing metadata a "warning", I think we could have a minimum set of minimum metadata (long-description, requires-python, etc) that "should" be included, and those could be warnings (errors on I'd like to see some sort of checker that does category 3 - I'd like something that makes sure I don't forget various classifiers, some types of URLs, etc. But I think just providing a bit of API to help others provide this is likely enough. A tool for this would need to support configuration to tell it what to allow / skip, etc. Similar to |
I need to briefly reply to this. Python version independent wheels predate What was developed as a standard was The proposal in a different issue was to simply check the Python tags, and produce a warning if a Python 3 only package (via Requires-Python) has a |
Well, that's probably because I can't say I really agree with the addition of the
This is covered in detail in pypa/packaging#147. The TL;DR is: move PyPI's metadata validation out of PyPI and into |
Yeah, I think a better name would have been akin to |
Caveat: I have very little experience with proposing and implementing substantial new features/APIs in open-source projects.
There have been more than a few requests to add functionality to
twine check
, but those have been mostly flagged as duplicates of #430, which is currently blocked on pypa/packaging#147, which hasn't seen much movement.#843 contains a proposal to add custom "checkers" via setuptools
entry_points
, which would allow functionality to be added by independently-maintained packages. Twine already allows for plugin sub-commands viaentry_points
, but those would have to implement argument parsing and reading packages from disk, and be run as a separate command fromtwine check
.The purpose of this issue is to gather requirements and propose an API for custom checkers. For an initial (but insufficient) overview of the current API, see the autodoc for
twine.commands.check
.Some initial ideas/questions:
long_description
, implemented in a private method. That should eventually be refactored to match the new API, but it could be refactored earlier to help establish the new API.PackageFile
, along with the original filename.check()
andmain()
intwine.commands.check
.The text was updated successfully, but these errors were encountered: