Allow "twine check" to run multiple checks #843
Conversation
Checkers are registered as entry points (twine.registered_checkers). A new "check_license" checker has been added.
There was a problem hiding this comment.
Thanks for making an effort to improve Twine. However, this seems like a non-trivial change in the application architecture, which I think should have been preceded by a proposal and discussion on the issue tracker. There are already a handful of issues related to making check more useful, and the thinking thus far has been that such logic should be implemented in the packaging library (see pypa/packaging#147), so that it could be used by multiple projects, including PyPI itself. It might be time to re-think that, but until then, I'm wary of "bolting on" additional functionality.
| twine.registered_checkers = | ||
| longdesc = twine.commands.check:check_long_description | ||
| license = twine.commands.check:check_license |
There was a problem hiding this comment.
What's the value in defining an entrypoint for registered_checkers? At first glance, it feels like an extra layer of complexity. Why not just call the check_* functions explicitly in twine.commands.check._check_file?
There was a problem hiding this comment.
Yes, calling the check_* functions directly would work as well. My goal here was to allow users to define project-specific checkers that may not be useful to all Python developers, or are far too strict to be included in twine itself.
| monkeypatch.setattr(check, "check", check_stub) | ||
|
|
||
| assert check.main(["dist/*"]) == check_result | ||
| assert check_stub.calls == [pretend.call(["dist/*"], strict=False)] |
There was a problem hiding this comment.
I appreciate the instinct to refactor, but it makes for a big diff that's hard to review. I probably would have just renamed the existing test functions to include the word description.
There was a problem hiding this comment.
Indeed, I only realized that when viewing the diff myself :-(
The tests have been slightly modified as well, since they used to be strongly tied to the "long description checking" logic. Now some tests focus on the long description checker, and some tests focus on the infrastructure, and are checker-agnostic.
| filename: str, render_warning_stream: _WarningStream | ||
| ) -> Tuple[List[str], bool]: | ||
| """Check given distribution.""" | ||
| def check_long_description(filename: str) -> Tuple[List[str], List[str]]: |
There was a problem hiding this comment.
These have effectively become a public API and - were we to merge this - must not live in twine.commands.
|
So I'm not actually opposed to this. We have a lot on the I'm vaguely +0 on the concept. To play to Brian's point, we already have entry-points that an organization could use to build its own checkers. You could register a command that looks like |
|
@CyrilRoelandteNovance Apologies for letting this languish. Based on your and @sigmavirus24's comments, I see the value in enabling custom checkers. However, before implementing that, I'd like to have an architecture discussion in a new issue, possibly soliciting opinions from folks who've asked for improvements to @sigmavirus24 How does that sound to you? |
Please do create a new issue but I'm not exactly sure what you expect to come from the architecture discussion. We don't have requirements for what to implement in the first place, how can we discuss architecture without what users need? Do you mean you'd like to discuss an API proposed here? I don't know that it warrants that until we know what people would need to implement the checks they want to implement. My point stands that today people can already "extend" twine. They can register their own commands and implement their own checks without us having to change what |
|
Caveat: I have very little experience with proposing and implementing substantial new features/APIs in open-source projects.
I was thinking that requirements gathering could be part of the discussion, using the API proposed here as an example.
I like the idea of plugging in functionality to As it stands, folks can write their own sub-commands, but they're on the hook for argument parsing and reading packages from disk. |
|
So Let's start a new issue, but again, I want to start from practical requirements and talk about what we can do to make this a pleasant experience versus the things learned the hard way in Flake8 |
|
Closing in favor of #848. |
Checkers are registered as entry points (twine.registered_checkers). A
new "check_license" checker has been added.