diff --git a/CHANGES.md b/CHANGES.md index 72c1fb6..743628b 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -3,7 +3,11 @@ ## 7.0.0b16 (unreleased) -- Nothing changed yet. +- Reject package names whose dotted parts are not valid Python identifiers, + instead of generating a package that cannot be installed. Applies to + templates that scaffold a Python package, and to the package name that will + actually be used -- a project template such as `zope-setup`, whose NAME is a + directory, is unaffected. [kunalKumar-13] ## 7.0.0b15 (2026-08-15) diff --git a/plonecli/cli.py b/plonecli/cli.py index bb53390..0f4b40d 100644 --- a/plonecli/cli.py +++ b/plonecli/cli.py @@ -3,12 +3,13 @@ from __future__ import annotations import importlib.metadata +import keyword import re import shutil import subprocess import sys import tomllib -from pathlib import Path +from pathlib import Path, PurePath import click @@ -20,6 +21,7 @@ from plonecli.registry import TemplateRegistry from plonecli.templates import ( ensure_templates_cloned, + get_template_path, get_templates_info, run_add, run_create, @@ -91,6 +93,69 @@ def _parse_data(pairs): return data +def _template_uses_package_name(reg, config, resolved): + """Whether this main template turns its answers into a Python package. + + Only some ``create`` templates do. ``backend_addon`` -- and ``addon``, the + composite that includes it -- ask a ``package_name`` question whose default + is the destination directory name, so the directory name has to be a legal + Python package. ``zope-setup`` asks ``project_name`` instead, where a + hyphen is ordinary and documented (``plonecli create zope-setup + my-project``). + + Read from the template rather than hardcoded, so a template added later is + classified by what it asks for instead of by a list someone has to + remember to update. A template that cannot be read is treated as not + asking, because refusing to scaffold on a failed lookup would be worse + than the bug this guards against. + """ + import yaml + + names = reg.get_composite_steps(resolved) or [resolved] + for step in names: + try: + questions = yaml.safe_load( + (get_template_path(step, config) / "copier.yml").read_text() + ) + except (OSError, yaml.YAMLError): + continue + if isinstance(questions, dict) and "package_name" in questions: + return True + return False + + +def _validate_package_name(name): + """Reject a package name whose dotted parts are not usable in Python. + + Each part becomes a Python package directory, so a dash, a leading digit or + a reserved word produces a package that cannot be imported or installed -- + see issue #72, where ``collective.new-testcase`` scaffolded a project that + ``pip install`` then rejected. ``name`` may be a path, in which case only + its last component is the package name. Raises ``click.BadParameter``. + """ + name = PurePath(name).name + parts = name.split(".") + bad = [p for p in parts if not p.isidentifier() or keyword.iskeyword(p)] + if not bad: + return + suggestion = name.replace("-", "_") + hint = "" + if suggestion != name and all( + p.isidentifier() and not keyword.iskeyword(p) for p in suggestion.split(".") + ): + hint = f" Try {suggestion!r}." + reason = ( + "is a Python keyword" + if all(keyword.iskeyword(p) for p in bad) + else "is not a valid Python identifier" + ) + raise click.BadParameter( + f"{name!r} is not a valid package name: " + f"{', '.join(repr(p) for p in bad)} {reason}.{hint}", + param_hint="NAME", + ) + + def _load_data_file(path): """Load copier answers from a YAML or JSON file into a dict. @@ -321,6 +386,14 @@ def create(context, template, name, data, data_file, defaults, no_git, allow_dir git_commit = config.auto_commit and not no_git answers = _collect_data(data_file, data) + # NAME is the output directory (`run_create(target_name=...)`). For a + # template that scaffolds a Python package it also DEFAULTS the package + # name, which is the case issue #72 is about -- but an explicit + # `-d package_name=` overrides that default, and a project template such as + # zope-setup never uses NAME as a package at all. Validate whichever value + # is actually going to be the package name, and only when there is one. + if _template_uses_package_name(reg, config, resolved): + _validate_package_name(answers.get("package_name") or name) steps = reg.get_composite_steps(resolved) if steps: echo(f"\nCreating {resolved} project: {name}", fg="green", reverse=True) diff --git a/tests/test_package_name_validation.py b/tests/test_package_name_validation.py new file mode 100644 index 0000000..22ff3ba --- /dev/null +++ b/tests/test_package_name_validation.py @@ -0,0 +1,143 @@ +"""Package name validation for the create command.""" + +import click +import pytest + +from plonecli.cli import _validate_package_name + + +@pytest.mark.parametrize( + "name", + [ + "collective.foo", + "collective.foo_bar", + "plone.app.contenttypes", + "myaddon", + ], +) +def test_valid_names_pass(name): + _validate_package_name(name) + + +@pytest.mark.parametrize( + "name", + [ + "collective.new-testcase", + "my-addon", + "collective.2foo", + "collective.class", + "collective.foo bar", + ], +) +def test_invalid_names_raise(name): + with pytest.raises(click.BadParameter): + _validate_package_name(name) + + +def test_dash_error_suggests_underscore(): + with pytest.raises(click.BadParameter) as exc: + _validate_package_name("collective.new-testcase") + assert "collective.new_testcase" in str(exc.value) + + +def test_error_names_the_offending_part(): + with pytest.raises(click.BadParameter) as exc: + _validate_package_name("collective.new-testcase") + assert "new-testcase" in str(exc.value) + + +def test_path_is_reduced_to_its_last_component(): + # The create command accepts a target path, not just a bare name. + _validate_package_name("/tmp/somewhere/collective.foo") + + +def test_path_with_invalid_last_component_still_raises(): + with pytest.raises(click.BadParameter): + _validate_package_name("/tmp/somewhere/collective.new-testcase") + + +def test_no_suggestion_when_underscore_would_not_help(): + with pytest.raises(click.BadParameter) as exc: + _validate_package_name("collective.2foo") + assert "Try" not in str(exc.value) + + +def test_keyword_error_says_keyword_not_identifier(): + # `class` IS a valid identifier -- it is reserved. Saying "not a valid + # Python identifier" sent the reader looking for a typo that is not there. + with pytest.raises(click.BadParameter) as exc: + _validate_package_name("collective.class") + assert "Python keyword" in str(exc.value) + + +# --------------------------------------------------------------------------- +# Which templates the check applies to. +# +# NAME is the OUTPUT DIRECTORY (`run_create(target_name=...)`), and only some +# templates turn it into a Python package. Validating it unconditionally +# rejected `plonecli create zope-setup my-project` -- straight out of the +# README -- and broke 26 of the 33 scaffolding evaluations, every one of which +# scaffolds into a hyphenated working directory. +# --------------------------------------------------------------------------- + + +class _Registry: + """Minimal stand-in: composite steps only, which is all the helper reads.""" + + def __init__(self, steps=None): + self._steps = steps or [] + + def get_composite_steps(self, resolved): + return self._steps + + +def _template(tmp_path, name, body): + d = tmp_path / name + d.mkdir(parents=True) + (d / "copier.yml").write_text(body) + return d + + +def _config_for(tmp_path, monkeypatch): + import plonecli.cli as cli + + monkeypatch.setattr(cli, "get_template_path", lambda n, c: tmp_path / n) + return object() + + +def test_package_template_is_detected(tmp_path, monkeypatch): + from plonecli.cli import _template_uses_package_name + + _template(tmp_path, "backend_addon", "package_name:\n type: str\n") + cfg = _config_for(tmp_path, monkeypatch) + assert _template_uses_package_name(_Registry(), cfg, "backend_addon") + + +def test_project_template_is_not(tmp_path, monkeypatch): + from plonecli.cli import _template_uses_package_name + + # zope-setup asks project_name, whose value is a directory name. + _template(tmp_path, "zope-setup", "project_name:\n type: str\n") + cfg = _config_for(tmp_path, monkeypatch) + assert not _template_uses_package_name(_Registry(), cfg, "zope-setup") + + +def test_composite_is_detected_through_its_steps(tmp_path, monkeypatch): + from plonecli.cli import _template_uses_package_name + + # `addon` has no copier questions of its own; it composes backend_addon. + _template(tmp_path, "addon", "_plonecli:\n type: composite\n") + _template(tmp_path, "backend_addon", "package_name:\n type: str\n") + _template(tmp_path, "zope-setup", "project_name:\n type: str\n") + cfg = _config_for(tmp_path, monkeypatch) + reg = _Registry(["backend_addon", "zope-setup"]) + assert _template_uses_package_name(reg, cfg, "addon") + + +def test_unreadable_template_does_not_block_scaffolding(tmp_path, monkeypatch): + from plonecli.cli import _template_uses_package_name + + # Nothing on disk. Refusing to scaffold because a lookup failed would be a + # worse failure than the one this guard exists to prevent. + cfg = _config_for(tmp_path, monkeypatch) + assert not _template_uses_package_name(_Registry(), cfg, "missing")