Validate package name in create

[kunalKumar-13]

Closes #72.

plonecli create addon collective.new-testcase generated a package that could not be installed — each dotted part becomes a Python package directory, so a dash produced an invalid module name and buildout failed later with an opaque EntryPoint error.

create now rejects the name up front:

Error: Invalid value for NAME: 'collective.new-testcase' is not a valid package name:
'new-testcase' is not a valid Python identifier. Try 'collective.new_testcase'.

The underscore suggestion only appears when it would actually produce a valid name, so collective.2foo gets the error without a misleading hint.

Went with rejecting rather than normalising, per the issue title — silently rewriting the name the user typed seemed worse than telling them. Happy to switch to normalisation if you'd rather have that.

One thing worth flagging: NAME can be a target path, not just a bare name — the existing tests pass things like /tmp/.../my.addon. Validation therefore only checks the last path component. I only found this because the existing suite caught it.

14 tests added. Full suite: 230 passed, 16 skipped. ruff check and ruff format --check clean on both files.

[Copilot]

🟡 Changes recommended

The new validation’s error message is inaccurate for keyword-based failures (and awkward for multiple invalid parts), which is user-facing and should be corrected before release.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR adds upfront validation for plonecli create package names to prevent generating projects whose dotted components can’t become importable Python package directories (e.g., components containing -, starting with digits, or being Python keywords), addressing issue #72.

Changes:

• Add _validate_package_name() and invoke it at the start of the create command.
• Provide a targeted underscore suggestion only when it results in a valid dotted name.
• Add a dedicated test module covering valid/invalid names, path inputs, and suggestion behavior; update CHANGES.md.

File summaries

File	Description
plonecli/cli.py	Introduces and wires in package-name validation for create, including improved CLI error feedback.
tests/test_package_name_validation.py	Adds unit tests for name/path validation and messaging behavior.
CHANGES.md	Documents the behavior change for the upcoming release.

Review details

Suppressed comments (1)

plonecli/cli.py:118

• The error message says the offending parts "is not a valid Python identifier", but bad also includes reserved keywords (e.g. class), which are valid identifiers. Also, if multiple parts are invalid, the singular wording becomes grammatically incorrect; consider building the message with proper pluralization and explicit keyword mention.

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

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[kunalKumar-13]

Good catch, fixed in 2a39588. The message now distinguishes the two cases: when every offending part is a keyword it says 'class' is a Python keyword rather than calling it an invalid identifier, which was misleading because class is a valid identifier and only reserved. The docstring says "a dash, a leading digit or a reserved word" now, and there's a test pinning the wording.

Same commit also fixes something bigger that the scaffolding job was catching: NAME is the output directory, not always a package name. backend_addon asks a package_name question defaulting to dst_path.name, but zope-setup asks project_name, where a hyphen is ordinary — plonecli create zope-setup my-project is in the README and this PR was rejecting it. The validator now checks the value that actually becomes the package name (an explicit -d package_name= when given, the directory otherwise) and only for templates that declare such a question, read from their copier.yml rather than a hardcoded list.

evals/scaffolding --quick went from 21 failures to 0.

[kunalKumar-13]

Disclosure I owe this PR: #79 by @arky is also open against #72 and I did not spot it when I opened this. It predates this by four years (May 2022).

The two are not the same change, which is why I am flagging rather than closing:

	approach
#79 (@arky)	a note in README.rst recommending people avoid dashes and underscores in package names
this PR	validates the name in create and fails with an explanatory message

Documentation and enforcement, not two attempts at one fix — and they compose fine, since the README note explains the rule that this now enforces.

I have no claim to precedence here. If maintainers prefer the documentation-only route, close this and take #79; it has been waiting a long time. If you would rather enforce it, this is ready, and #79's wording would still be a good addition on top.

@arky apologies for arriving on top of your PR without acknowledging it.

[kunalKumar-13]

Correcting myself: there is a third PR against #72 that I missed, and it matters more than #79.

@arky also opened #77, "Check project name for special chars", on the same day in May 2022 — the same validation approach as this PR. He closed it himself within the day, with no maintainer comment, calling it "a quick and dirty fix", and opened the README note in #79 instead.

Worth knowing why it did not work, because it is the reason validation looked like a dead end in 2022. #77 rejected any name containing punctuation:

if set(name).intersection(set(string.punctuation)):
    raise NameError('Name contains special characters')

A dot is punctuation, and dotted names are the Plone convention. Running both rules over real names:

name	#77 (2022)	this PR
collective.todo	REJECT	accept
plone.app.content	REJECT	accept
my_addon	REJECT	accept
src/collective.todo	REJECT	accept
collective.new-testcase	REJECT	REJECT
collective.class	REJECT	REJECT
2cool	accept	REJECT

So #77 rejected essentially every valid Plone package name while still letting 2cool through. Abandoning it was the right call.

This PR splits on dots and asks whether each part is a Python identifier and not a keyword, which is the actual rule — each part becomes an importable directory. That admits the dotted convention, still catches the collective.new-testcase case from #72, and additionally catches a leading digit and reserved words. It also only applies to templates that ask a package_name question, so plonecli create zope-setup my-project keeps working, since there the name is a directory rather than a package.

None of this is a criticism of @arky — he found the bug, filed it, and tried the fix four years before I did. It is context for choosing between the three: #79 documents the rule, this enforces it, and #77 shows the shape of enforcement to avoid.