From 897d5a078146fc38bc3e9b437e29605ea3080a4c Mon Sep 17 00:00:00 2001 From: Kunal Kumar Date: Thu, 3 Sep 2026 00:08:00 +0530 Subject: [PATCH 1/2] Validate package name in create Each dotted part becomes a Python package directory, so a dash or a leading digit produced a package that could not be installed. Fail with a clear message instead, suggesting the underscore form where that would be valid. Only the last path component is checked, since NAME may be a target path. Closes #72 --- CHANGES.md | 3 +- plonecli/cli.py | 30 ++++++++++++- tests/test_package_name_validation.py | 62 +++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 2 deletions(-) create mode 100644 tests/test_package_name_validation.py diff --git a/CHANGES.md b/CHANGES.md index 72c1fb6..6458518 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -3,7 +3,8 @@ ## 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. [kunalKumar-13] ## 7.0.0b15 (2026-08-15) diff --git a/plonecli/cli.py b/plonecli/cli.py index bb53390..3496095 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 @@ -91,6 +92,32 @@ def _parse_data(pairs): return data +def _validate_package_name(name): + """Reject names whose dotted parts are not valid Python identifiers. + + Each part becomes a Python package directory, so a dash or a leading digit + produces a package that cannot be imported or installed. ``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}." + raise click.BadParameter( + f"{name!r} is not a valid package name: " + f"{', '.join(repr(p) for p in bad)} is not a valid Python identifier.{hint}", + param_hint="NAME", + ) + + def _load_data_file(path): """Load copier answers from a YAML or JSON file into a dict. @@ -303,6 +330,7 @@ def format_help(self, ctx, formatter): @click.pass_context def create(context, template, name, data, data_file, defaults, no_git, allow_dirty): """Create a new Plone package""" + _validate_package_name(name) config = context.obj["config"] ensure_templates(config) reg = TemplateRegistry(config) diff --git a/tests/test_package_name_validation.py b/tests/test_package_name_validation.py new file mode 100644 index 0000000..120d5a5 --- /dev/null +++ b/tests/test_package_name_validation.py @@ -0,0 +1,62 @@ +"""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) From 2a39588c4002746afa97733cb990571ae6796591 Mon Sep 17 00:00:00 2001 From: Kunal Kumar Date: Thu, 3 Sep 2026 21:10:45 +0530 Subject: [PATCH 2/2] Validate the package name, not the target directory NAME is the output directory -- run_create() takes it as target_name and hands it to copier as dst_path. Only some templates turn it into a Python package: backend_addon asks a package_name question defaulting to dst_path.name, and addon is a composite that includes it. zope-setup asks project_name instead, where a hyphen is ordinary and documented (`plonecli create zope-setup my-project`, README). Validating NAME unconditionally therefore rejected legitimate input. It refused that README example, and it failed 21 of the scaffolding evaluations in --quick and 26 in --ci-validation, every one of which scaffolds into a hyphenated workspace directory. Validate the value that will actually become the package name -- an explicit `-d package_name=` when given, the directory name otherwise -- and only for templates that ask for one. Whether a template asks is read from its copier.yml rather than hardcoded, so a template added later is classified by what it declares. A template that cannot be read is treated as not asking: refusing to scaffold because a lookup failed is worse than the bug this guards against. Issue #72's case is unaffected -- `create addon collective.new-testcase` is still rejected. Also: say "is a Python keyword" rather than "is not a valid Python identifier" when every offending part is a keyword. `class` IS a valid identifier, it is reserved, and the old wording sent the reader looking for a typo that was not there. evals --quick: 21 failures before, 0 after. 235 tests pass, ruff clean. --- CHANGES.md | 5 +- plonecli/cli.py | 59 ++++++++++++++++--- tests/test_package_name_validation.py | 81 +++++++++++++++++++++++++++ 3 files changed, 137 insertions(+), 8 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 6458518..743628b 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,7 +4,10 @@ - Reject package names whose dotted parts are not valid Python identifiers, - instead of generating a package that cannot be installed. [kunalKumar-13] + 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 3496095..0f4b40d 100644 --- a/plonecli/cli.py +++ b/plonecli/cli.py @@ -21,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, @@ -92,13 +93,45 @@ 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 names whose dotted parts are not valid Python identifiers. + """Reject a package name whose dotted parts are not usable in Python. - Each part becomes a Python package directory, so a dash or a leading digit - produces a package that cannot be imported or installed. ``name`` may be a - path, in which case only its last component is the package name. Raises - ``click.BadParameter``. + 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(".") @@ -111,9 +144,14 @@ def _validate_package_name(name): 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)} is not a valid Python identifier.{hint}", + f"{', '.join(repr(p) for p in bad)} {reason}.{hint}", param_hint="NAME", ) @@ -330,7 +368,6 @@ def format_help(self, ctx, formatter): @click.pass_context def create(context, template, name, data, data_file, defaults, no_git, allow_dirty): """Create a new Plone package""" - _validate_package_name(name) config = context.obj["config"] ensure_templates(config) reg = TemplateRegistry(config) @@ -349,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 index 120d5a5..22ff3ba 100644 --- a/tests/test_package_name_validation.py +++ b/tests/test_package_name_validation.py @@ -60,3 +60,84 @@ 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")