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 all 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",

# 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
74 changes: 50 additions & 24 deletions twine/commands/check.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,16 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import argparse
import dataclasses
import email.message
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 valid_classifiers

from twine import commands
from twine import package as package_file
Expand Down Expand Up @@ -74,21 +76,31 @@ def _parse_content_type(value: str) -> Tuple[str, Dict[str, str]]:
return msg.get_content_type(), msg["content-type"].params


@dataclasses.dataclass
class CheckedFileResults:
warnings: List[str]
errors: List[str]

@property
def is_ok(self) -> bool:
return len(self.errors) == 0


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?

) -> CheckedFileResults:
"""Check given distribution."""
warnings = []
is_ok = True
result = CheckedFileResults(warnings=[], 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"])

if description_content_type is None:
warnings.append(
result.warnings.append(
"`long_description_content_type` missing. defaulting to `text/x-rst`."
)
description_content_type = "text/x-rst"
Expand All @@ -97,30 +109,41 @@ def _check_file(
renderer = _RENDERERS.get(content_type, _RENDERERS[None])

if description is None or description.rstrip() == "UNKNOWN":
warnings.append("`long_description` missing.")
result.warnings.append("`long_description` missing.")
elif renderer:
rendering_result = renderer.render(
description, stream=render_warning_stream, **params
)
if rendering_result is None:
is_ok = False
result.errors.append(
"`long_description` has syntax errors in markup"
" and would not be rendered on PyPI."
)

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

return warnings, is_ok
return result


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,18 +160,15 @@ def check(
for filename in uploads:
print(f"Checking {filename}: ", end="")
render_warning_stream = _WarningStream()
warnings, is_ok = _check_file(filename, render_warning_stream)
check_result = _check_file(filename, render_warning_stream)

# Print the status and/or error
if not is_ok:
if not check_result.is_ok:
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}"
)
elif warnings:
error_mgs = "\n".join(check_result.errors)
logger.error(f"{error_mgs}\n" f"{render_warning_stream}")
elif check_result.warnings:
if strict:
failure = True
print("[red]FAILED due to warnings[/red]")
Expand All @@ -158,7 +178,7 @@ def check(
print("[green]PASSED[/green]")

# Print warnings after the status and/or error
for message in warnings:
for message in check_result.warnings:
logger.warning(message)

return failure
Expand All @@ -173,7 +193,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
Loading