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

Validate Trove classifiers in twine check #1166

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ dependencies = [
"keyring >= 15.1; platform_machine != 'ppc64le' and platform_machine != 's390x'",
"rfc3986 >= 1.4.0",
"rich >= 12.0.0",
"trove-classifiers >= 2024.10.12 ",
browniebroke marked this conversation as resolved.
Show resolved Hide resolved

# workaround for #1116
"pkginfo < 1.11",
Expand Down
74 changes: 74 additions & 0 deletions tests/test_check.py
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,80 @@ def test_main(monkeypatch):
assert check_stub.calls == [pretend.call(["dist/*"], strict=False)]


def test_fails_invalid_classifiers(tmp_path, capsys, caplog):
sdist = build_sdist(
tmp_path,
{
"setup.cfg": (
"""
[metadata]
name = test-package
version = 0.0.1
long_description = file:README.md
long_description_content_type = text/markdown
classifiers =
Framework :: Django :: 5
"""
),
"README.md": (
"""
# test-package

A test package.
"""
),
},
)

assert check.check([sdist])

assert capsys.readouterr().out == f"Checking {sdist}: FAILED\n"

assert len(caplog.record_tuples) > 0
assert caplog.record_tuples == [
(
"twine.commands.check",
logging.ERROR,
"`Framework :: Django :: 5` is not a valid classifier"
" and would prevent upload to PyPI.\n",
)
]


def test_passes_valid_classifiers(tmp_path, capsys, caplog):
sdist = build_sdist(
tmp_path,
{
"setup.cfg": (
"""
[metadata]
name = test-package
version = 0.0.1
long_description = file:README.md
long_description_content_type = text/markdown
classifiers =
Programming Language :: Python :: 3
Framework :: Django
Framework :: Django :: 5.1
"""
),
"README.md": (
"""
# test-package

A test package.
"""
),
},
)

assert not check.check([sdist])

assert capsys.readouterr().out == f"Checking {sdist}: PASSED\n"

assert caplog.record_tuples == []


# TODO: Test print() color output

# TODO: Test log formatting
54 changes: 35 additions & 19 deletions twine/commands/check.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@
import io
import logging
import re
from typing import Dict, List, Optional, Tuple, cast
from typing import Dict, List, Optional, Sequence, Tuple, cast

import readme_renderer.rst
from rich import print
from trove_classifiers import classifiers as all_classifiers
browniebroke marked this conversation as resolved.
Show resolved Hide resolved

from twine import commands
from twine import package as package_file
Expand Down Expand Up @@ -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]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

) -> Tuple[List[str], List[str]]:
"""Check given distribution."""
warnings = []
is_ok = True
errors = []

package = package_file.PackageFile.from_filename(filename, comment=None)

metadata = package.metadata_dictionary()

# Check description
description = cast(Optional[str], metadata["description"])
description_content_type = cast(Optional[str], metadata["description_content_type"])

Expand All @@ -103,24 +105,35 @@ def _check_file(
description, stream=render_warning_stream, **params
)
if rendering_result is None:
is_ok = False
errors.append(
"`long_description` has syntax errors in markup"
" and would not be rendered on PyPI."
)

return warnings, is_ok
# Check classifiers
dist_classifiers = cast(Sequence[str], metadata["classifiers"])
for classifier in dist_classifiers:
if classifier not in all_classifiers:
errors.append(
f"`{classifier}` is not a valid classifier"
f" and would prevent upload to PyPI."
)

return warnings, errors


def check(
dists: List[str],
strict: bool = False,
) -> bool:
"""Check that a distribution will render correctly on PyPI and display the results.
"""Check that a distribution will upload and render correctly on PyPI.

This is currently only validates ``long_description``, but more checks could be
added.
This currently validates
- ``long_description``, to make sure it would render correctly on PyPI
- ``classifiers``, to make sure that all classifiers are valid

:param dists:
The distribution files to check.
:param output_stream:
The destination of the resulting output.
:param strict:
If ``True``, treat warnings as errors.

Expand All @@ -137,17 +150,14 @@ def check(
for filename in uploads:
print(f"Checking {filename}: ", end="")
render_warning_stream = _WarningStream()
warnings, is_ok = _check_file(filename, render_warning_stream)
warnings, errors = _check_file(filename, render_warning_stream)

# Print the status and/or error
if not is_ok:
if errors:
failure = True
print("[red]FAILED[/red]")
logger.error(
"`long_description` has syntax errors in markup"
" and would not be rendered on PyPI."
f"\n{render_warning_stream}"
)
error_mgs = "\n".join(errors)
logger.error(f"{error_mgs}\n" f"{render_warning_stream}")
elif warnings:
if strict:
failure = True
Expand All @@ -173,7 +183,13 @@ def main(args: List[str]) -> bool:
:return:
The exit status of the ``check`` command.
"""
parser = argparse.ArgumentParser(prog="twine check")
parser = argparse.ArgumentParser(
prog="twine check",
description=(
"Check distribution files and make sure they will upload and render"
" correctly on PyPI. Validates description and all classifiers."
),
)
parser.add_argument(
"dists",
nargs="+",
Expand Down