Validate Trove classifiers in twine check #1166
Conversation
browniebroke
force-pushed
the
twine-check-classifiers
branch
3 times, most recently
from
6530e6d to
933a503
Compare
browniebroke
force-pushed
the
twine-check-classifiers
branch
from
933a503 to
1beae6c
Compare
| @@ -76,14 +77,15 @@ def _parse_content_type(value: str) -> Tuple[str, Dict[str, str]]: | |||
|
|
|||
| def _check_file( | |||
| filename: str, render_warning_stream: _WarningStream | |||
| ) -> Tuple[List[str], bool]: | |||
There was a problem hiding this comment.
That we're changing this signature is a sign that we need to do more here.
Check file previously just cared about the readme and to a lesser extent the metadata version.
I'd argue we should instead have check readme and check classifiers functions. I'd also argue for a structured data object for representing a finding, the severity and confidence instead of returning lists of strings.
There was a problem hiding this comment.
As far as I understand, _check_file takes the whole distribution file name as argument (e.g. dist/test-package-0.0.1.tar.gz), extracts the metadata from it, and run some check on these. AFAIU, it does not seem to read the readme file, the description comes from the metadata as RestructuredText/Markdown, and it tries to render it as HTML.
Wouldn't a function called _check_readme be misleading? Can the description be inlined? I need to read more about the spec...
A structured data object representing the result makes sense to me, I'm not a big fan of returning tuples, even less tuples of list/dict. Sounds like a good candidate would be a dataclass or typing.NamedTuple, I think either can be used here as Twine currently supports Python 3.8+. Any preference?
|
I don't believe this is the reusable library referenced in the original issue. Maybe I'm wrong though |
|
Oh, did you mean that last comment:
|
|
Ok, I didn't pay enough attention to the time line of all the events across issue comments and PRs merge. It seems like the |
|
Tried to use I don't think it validates classifiers (yet?), and the description rendering problem aren't caught, so it looks like we won't be able to strip out so much from twine after all. |
I've been bitten by #976 today while trying to use
Framework :: Django :: 5as classifier. It used to work with earlier versions of Django (e.g.Framework :: Django :: 4) and the package in question was using these, so it wasn't clear that just 5 wouldn't work.Reading the conversation in the issue, it sems like folks are happy to add this feature to Twine so I went ahead a tried to do it.
Fix #976