Validate package name in create

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)

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)

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")