feat(ext.generate): add prompt_mode: select for features (#779)#780
Closed
derks wants to merge 4 commits into
Closed
feat(ext.generate): add prompt_mode: select for features (#779)#780derks wants to merge 4 commits into
derks wants to merge 4 commits into
Conversation
Adds an opt-in multi-valued feature prompt to .generate.yml templates. Today a feature is boolean-like and dispatches to enabled:/disabled:; this adds a `prompt_mode: select` that presents a numbered picker and dispatches the chosen value into one of N `options` branches. Implementation delegates to cement.utils.shell.Prompt rather than building parallel infrastructure: Meta.options + Meta.numbered=True + Meta.case_insensitive=True + Meta.default cover the whitelist, display, matching, and fallback behavior end-to-end. The numbered display returns the matched option label string, which the merge step maps back to the underlying value via positional zip. New optional schema: - `prompt_mode: select` opts in. The default is `'boolean'` (legacy semantics, byte-identical when key absent — `prompt_mode: boolean` and omission are exactly equivalent). - `default:` is required for select-mode and must be in the option values (validated at config-load). - `options:` is a list of dicts; each has a required `value:` and an optional `prompt:` label (falls back to str(value)). Each option may also carry `ignore` / `exclude` / `variables` (same vocabulary as `enabled:` / `disabled:`). - Silent (no-prompt) variables under any branch via `prompt: false` sentinel; `case` and `validate` still apply; str-coerced so YAML- decoded non-string defaults (bool, int) don't crash downstream case/validate operations. - Fail-fast validation: ValueError on prompt_mode whitelist miss, `select` + enabled/disabled collision, missing/empty `options`, missing `value:` in option, missing `default:`, default not in option values, duplicate option labels. Backward compat absolute: every pre-#779 .generate.yml walks the legacy `else`-branch byte-identically. 15 pre-existing fixtures (test1..test15) pass unchanged. Generalizes commit 6895aa5's `or []` null-safety idiom from the top-level YAML keys to the per-block keys on both the legacy and select-mode merge paths. 15 new tests + 15 new fixture directories (test16..test30) cover happy paths, every validator branch, silent variables, the requires cascade, and the null-coalesce regressions. cement.ext.ext_generate coverage stays 100% (202/202 stmts). ruff + mypy green. audit-public-api: empty diff vs Phase 03 baseline. Resolves #779. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extend the generate-features demo to exercise both prompt modes side-by-side. Existing `docker` / `docker_compose` boolean features stay byte-identical aside from gaining an explicit `prompt_mode: boolean` so the canonical living example demonstrates both styles (omission and explicit form are exactly equivalent). New `web_framework` select-mode feature with three options: - value: "none" (default) — ignores requirements.txt and wsgi.py - value: "flask" — silent framework_version=3.0, ignores fastapi - value: "fastapi" — silent framework_version=0.115, ignores flask Two new template files (requirements.txt, wsgi.py) land additively so the options branches' ignore patterns have real files to suppress. The README "Multi-Valued Feature (prompt_mode: select)" section walks through the schema keys, the case-insensitive numbered picker UX, silent variables, the --defaults headless path, and the requires: cascade interaction (a disabled select feature contributes nothing to the merge). End-to-end verified: `pdm run python myapp.py generate webapp /tmp --defaults` renders 5 expected files (no requirements.txt / wsgi.py per web_framework=none default). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
One entry under active 3.0.15 - DEVELOPMENT / Features bucket for GH issue #779. Notes that `prompt_mode` defaults to `boolean` (legacy behavior, byte-identical when key absent). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Quick task 260524-g47 — prompt_mode: select for ext.generate features (resolves #779). Captures the planning artifacts from a --full quick-task run that pivoted mid-flight from a hand-rolled prompt/validation implementation to delegating to shell.Prompt. Artifacts: - CONTEXT.md (re-locked decisions post-pivot; cites shell.Prompt as the dispatch primitive) - PLAN.md (first-attempt plan; preserved as historical record) - RESEARCH.md (first-attempt grafting notes; preserved) - REVIEW.md (first-attempt code-review findings; preserved) - SUMMARY.md (rebuild outcome — actual commit shape, gate results, coverage map, adjudication dispositions) - VERIFICATION.md (first-attempt verification; preserved) STATE.md: added the Status column to the Quick Tasks Completed table (first --full quick task with verifier on this project), appended the new row pointing at feat commit 0a8ffcb, refreshed Last Activity. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR extends the cement generate extension with multi-valued optional feature support. It adds ChangesMulti-valued Optional Feature Support (prompt_mode: select)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
@.planning/todos/completed/2026-05-08-analyze-and-review-pr-768-generate-optional-features.md:
- Line 22: Replace the malformed ATX heading token "`#743`." with plain text issue
notation so markdownlint MD018 stops triggering; locate the line containing the
literal "`#743`." and change it to something like "Issue 743." or "Fixes `#743`."
(i.e., ensure there is no leading '#' immediately followed by digits).
In `@cement/ext/ext_generate.py`:
- Around line 290-293: Replace the assert with an explicit runtime check that
raises a clear exception (e.g., ValueError) when a variable with "prompt": False
has no default: check if var['default'] is None and raise ValueError(f"Variable
'{var['name']}' has prompt: false but no default") instead of using assert, and
only call val = str(var['default']) after this validation so you never call
str(None) even when Python is run with -O.
- Around line 149-173: The SelectPrompt is setting Meta.default to the label
string (default_label) but with numbered=True the prompt expects a 1-based
numeric default; change Meta.default to the 1-based index of the chosen option
(e.g., compute default_index = labels.index(default) + 1 and set Meta.default to
that numeric value or its string) and ensure you only compute this if default is
present in labels (fall back to None or omit default if not found); update the
code around prompt_text/SelectPrompt/chosen_label to use that numeric default
value.
In `@CHANGELOG.md`:
- Around line 172-179: The changelog entry for `[ext.generate]` is spread over
multiple lines; collapse it into a single-line bullet in the active development
section by replacing the multi-line block with one line that starts with
`[ext.generate]` and includes the essential text (mention `prompt_mode: select`,
`cement.utils.shell.Prompt`, and that `options` branches may declare
`ignore`/`exclude`/`variables`), keeping it concise and preserving the Issue
reference `Issue `#779``; ensure the line follows the one-line-per-entry rule and
retains the same information but compressed into a single sentence.
In `@demo/generate-features/README.md`:
- Around line 112-120: The fenced code block showing the "Web Framework"
selection is missing a language tag; update that triple-backtick fence to
include a language identifier (use "text") so it becomes ```text at the start of
the block that contains "Web Framework" through "Enter the number for your
selection:" (the fenced block in demo/generate-features/README.md).
In `@tests/ext/test_ext_generate.py`:
- Around line 372-556: All listed test functions (e.g.,
test_generate_features_select_defaults, test_generate_features_select_collision,
..., test_generate_features_select_duplicate_labels) need type annotations; add
a parameter annotation for tmp (tmp: Any) and a return annotation -> None on
each function, and add "from typing import Any" to the file imports so the
symbol Any is available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 336f9e11-33d2-4797-a456-c332acca6d7d
📒 Files selected for processing (45)
.planning/STATE.md.planning/todos/completed/2026-05-08-analyze-and-review-pr-768-generate-optional-features.md.planning/todos/pending/2026-05-09-document-optional-features-in-gitbook-post-3-0-16.mdCHANGELOG.mdMakefilecement/ext/ext_generate.pydemo/generate-features/README.mddemo/generate-features/templates/generate/webapp/.generate.ymldemo/generate-features/templates/generate/webapp/requirements.txtdemo/generate-features/templates/generate/webapp/wsgi.pytests/data/templates/generate/test16/.generate.ymltests/data/templates/generate/test16/branch-1-onlytests/data/templates/generate/test16/branch-2-onlytests/data/templates/generate/test16/branch-N-filetests/data/templates/generate/test16/take-metests/data/templates/generate/test17/.generate.ymltests/data/templates/generate/test17/take-metests/data/templates/generate/test18/.generate.ymltests/data/templates/generate/test18/take-metests/data/templates/generate/test19/.generate.ymltests/data/templates/generate/test19/take-metests/data/templates/generate/test20/.generate.ymltests/data/templates/generate/test20/take-metests/data/templates/generate/test21/.generate.ymltests/data/templates/generate/test21/take-metests/data/templates/generate/test22/.generate.ymltests/data/templates/generate/test22/take-me-silenttests/data/templates/generate/test23/.generate.ymltests/data/templates/generate/test23/take-metests/data/templates/generate/test24/.generate.ymltests/data/templates/generate/test24/take-metests/data/templates/generate/test25/.generate.ymltests/data/templates/generate/test25/should-not-appear-1tests/data/templates/generate/test25/take-metests/data/templates/generate/test26/.generate.ymltests/data/templates/generate/test26/take-metests/data/templates/generate/test27/.generate.ymltests/data/templates/generate/test27/take-metests/data/templates/generate/test28/.generate.ymltests/data/templates/generate/test28/take-metests/data/templates/generate/test29/.generate.ymltests/data/templates/generate/test29/take-metests/data/templates/generate/test30/.generate.ymltests/data/templates/generate/test30/take-metests/ext/test_ext_generate.py
| and `ignore` patterns, and dependency resolution via `requires` with | ||
| transitive cascading. Also includes a fix to `setup_template_items` to catch | ||
| `ImportError` and a default-value correction (`{}` → `[]`). Resolves issue | ||
| #743. |
There was a problem hiding this comment.
Fix malformed markdown token that triggers MD018.
Line 22 begins with #743. and is parsed as an ATX heading without a required space.
Use plain text issue notation instead to avoid markdownlint failure.
Proposed fix
-#743.
+issue `#743`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #743. | |
| issue `#743`. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 22-22: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
@.planning/todos/completed/2026-05-08-analyze-and-review-pr-768-generate-optional-features.md
at line 22, Replace the malformed ATX heading token "`#743`." with plain text
issue notation so markdownlint MD018 stops triggering; locate the line
containing the literal "`#743`." and change it to something like "Issue 743." or
"Fixes `#743`." (i.e., ensure there is no leading '#' immediately followed by
digits).
Comment on lines
+149
to
+173
| default_label = ( # pragma: nocover | ||
| labels[values.index(default)] | ||
| ) | ||
| # Format: leading \n visually separates the | ||
| # numbered list from any prior prompt's output; | ||
| # trailing ':' matches the boolean feature prompt | ||
| # (line 175) and variable prompt (line 296) which | ||
| # both hardcode `:` at the end of their format | ||
| # strings. Template authors should NOT include | ||
| # trailing punctuation in their `prompt:` field | ||
| # (same convention as variables). | ||
| prompt_text = ( # pragma: nocover | ||
| feature.get('prompt') or f"Select Feature: {name}" | ||
| ) | ||
| prompt_text = f"\n{prompt_text}:" # pragma: nocover | ||
|
|
||
| class SelectPrompt(shell.Prompt): # pragma: nocover | ||
| class Meta: | ||
| text = prompt_text | ||
| options = labels | ||
| numbered = True | ||
| case_insensitive = True | ||
| default = default_label | ||
|
|
||
| chosen_label = SelectPrompt().prompt() # pragma: nocover |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Prompt numbered/default behavior and this callsite wiring.
rg -n -C6 'numbered|default|self\._meta\.options\[int\(self\.input\) - 1\]' cement/utils/shell.py
rg -n -C8 'class SelectPrompt|numbered = True|default =' cement/ext/ext_generate.pyRepository: datafolklabs/cement
Length of output: 11357
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="cement/ext/ext_generate.py"
echo "== default/numbered select prompt section =="
nl -ba "$FILE" | sed -n '110,210p'
echo "== variable prompt:false / assert section =="
nl -ba "$FILE" | sed -n '250,340p'
echo "== any assert usage in this file =="
rg -n "assert " "$FILE"Repository: datafolklabs/cement
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="cement/ext/ext_generate.py"
echo "== default/numbered select prompt section =="
cat -n "$FILE" | sed -n '110,210p'
echo "== variable prompt:false / assert section =="
cat -n "$FILE" | sed -n '250,340p'
echo "== any assert usage in this file =="
rg -n "assert " "$FILE" || trueRepository: datafolklabs/cement
Length of output: 10494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="cement/utils/shell.py"
# Show the exact logic around default application and numbered option parsing
sed -n '460,540p' "$FILE" | cat -nRepository: datafolklabs/cement
Length of output: 3641
Use a numeric default for numbered select prompts.
shell.Prompt with numbered=True treats the entered/default value as a 1-based index (options[int(self.input) - 1]). cement/ext/ext_generate.py currently sets SelectPrompt.Meta.default to the option label (default_label), so pressing Enter feeds a non-numeric string into that conversion, causing retries and potentially None/mapping failures.
Proposed fix
- default_label = ( # pragma: nocover
- labels[values.index(default)]
- )
+ default_idx = values.index(default) + 1 # pragma: nocover
@@
class SelectPrompt(shell.Prompt): # pragma: nocover
class Meta:
text = prompt_text
options = labels
numbered = True
case_insensitive = True
- default = default_label
+ default = str(default_idx)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cement/ext/ext_generate.py` around lines 149 - 173, The SelectPrompt is
setting Meta.default to the label string (default_label) but with numbered=True
the prompt expects a 1-based numeric default; change Meta.default to the 1-based
index of the chosen option (e.g., compute default_index = labels.index(default)
+ 1 and set Meta.default to that numeric value or its string) and ensure you
only compute this if default is present in labels (fall back to None or omit
default if not found); update the code around
prompt_text/SelectPrompt/chosen_label to use that numeric default value.
Comment on lines
+290
to
+293
| assert var['default'] is not None, \ | ||
| f"Variable '{var['name']}' has prompt: false " \ | ||
| f"but no default" | ||
| val = str(var['default']) |
There was a problem hiding this comment.
Replace assert-based config validation with an explicit exception.
Line 290 uses assert for required config. Under python -O, this check is removed and invalid config can slip through (str(None)).
Proposed fix
- assert var['default'] is not None, \
- f"Variable '{var['name']}' has prompt: false " \
- f"but no default"
+ if var['default'] is None:
+ raise ValueError(
+ f"Variable '{var['name']}' has prompt: false but no default"
+ )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cement/ext/ext_generate.py` around lines 290 - 293, Replace the assert with
an explicit runtime check that raises a clear exception (e.g., ValueError) when
a variable with "prompt": False has no default: check if var['default'] is None
and raise ValueError(f"Variable '{var['name']}' has prompt: false but no
default") instead of using assert, and only call val = str(var['default']) after
this validation so you never call str(None) even when Python is run with -O.
Comment on lines
+172
to
+179
| - `[ext.generate]` Add `prompt_mode: select` for multi-valued feature | ||
| prompts. A select-mode feature presents a numbered picker (delegated | ||
| to `cement.utils.shell.Prompt`) and dispatches the chosen value into | ||
| one of N `options` branches — each branch may declare its own | ||
| `ignore` / `exclude` / `variables` (incl. silent `prompt: false` | ||
| variables). `prompt_mode` defaults to `boolean` (legacy behavior, | ||
| byte-identical when key absent). | ||
| - [Issue #779](https://github.com/datafolklabs/cement/issues/779) |
There was a problem hiding this comment.
Keep this new changelog item to a single line.
This entry is wrapped across multiple lines; collapse it into one [ext.generate] bullet in the active development section.
As per coding guidelines, "Each CHANGELOG.md entry must be one line, prefixed with [area] (e.g., [ext.smtp], [cli], [dev], [core.handler])."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 172 - 179, The changelog entry for
`[ext.generate]` is spread over multiple lines; collapse it into a single-line
bullet in the active development section by replacing the multi-line block with
one line that starts with `[ext.generate]` and includes the essential text
(mention `prompt_mode: select`, `cement.utils.shell.Prompt`, and that `options`
branches may declare `ignore`/`exclude`/`variables`), keeping it concise and
preserving the Issue reference `Issue `#779``; ensure the line follows the
one-line-per-entry rule and retains the same information but compressed into a
single sentence.
Comment on lines
+112
to
+120
| ``` | ||
| Web Framework | ||
|
|
||
| 1: None — no web framework | ||
| 2: Flask Web Framework | ||
| 3: FastAPI Web Framework | ||
|
|
||
| Enter the number for your selection: | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to this fenced code block.
The block starting at Line 112 is missing a language identifier (MD040).
Proposed fix
-```
+```text
Web Framework
@@
Enter the number for your selection:
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| Web Framework | |
| 1: None — no web framework | |
| 2: Flask Web Framework | |
| 3: FastAPI Web Framework | |
| Enter the number for your selection: | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 112-112: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@demo/generate-features/README.md` around lines 112 - 120, The fenced code
block showing the "Web Framework" selection is missing a language tag; update
that triple-backtick fence to include a language identifier (use "text") so it
becomes ```text at the start of the block that contains "Web Framework" through
"Enter the number for your selection:" (the fenced block in
demo/generate-features/README.md).
Comment on lines
+372
to
+556
| def test_generate_features_select_defaults(tmp): | ||
| # test16: prompt_mode: select with three options; --defaults | ||
| # dispatches to the feature default ("N") and applies that | ||
| # branch's ignore patterns (suppresses branch-1-only, | ||
| # branch-2-only); branch-N-file and take-me remain. | ||
| argv = ['generate', 'test16', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| app.run() | ||
|
|
||
| assert exists_join(tmp.dir, 'take-me') | ||
| assert exists_join(tmp.dir, 'branch-N-file') | ||
| assert not exists_join(tmp.dir, 'branch-1-only') | ||
| assert not exists_join(tmp.dir, 'branch-2-only') | ||
|
|
||
|
|
||
| def test_generate_features_select_collision(tmp): | ||
| # test17: prompt_mode: select + enabled:/disabled: blocks on the | ||
| # same feature → ValueError at config validation. | ||
| argv = ['generate', 'test17', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| with raises(ValueError, | ||
| match="'enabled'/'disabled' blocks are not allowed"): | ||
| app.run() | ||
|
|
||
|
|
||
| def test_generate_features_select_invalid_prompt_mode(tmp): | ||
| # test18: prompt_mode: bogus → ValueError (whitelist enforced). | ||
| argv = ['generate', 'test18', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| with raises(ValueError, match="invalid prompt_mode 'bogus'"): | ||
| app.run() | ||
|
|
||
|
|
||
| def test_generate_features_select_silent_variable(tmp): | ||
| # test19: prompt_mode: select; --defaults dispatches into the "1" | ||
| # branch which carries a silent variable `chosen_version: v1-silent` | ||
| # (prompt: false). The silent variable's default lands verbatim in | ||
| # the rendered output. | ||
| argv = ['generate', 'test19', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| app.run() | ||
|
|
||
| assert exists_join(tmp.dir, 'take-me') | ||
| with open(os.path.join(tmp.dir, 'take-me')) as f: | ||
| assert 'v1-silent' in f.read() | ||
|
|
||
|
|
||
| def test_generate_features_select_default_not_in_values(tmp): | ||
| # test20: feature default "X" but options only contains "Y" → | ||
| # ValueError at config validation. | ||
| argv = ['generate', 'test20', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| with raises(ValueError, match="is not in options values"): | ||
| app.run() | ||
|
|
||
|
|
||
| def test_generate_features_select_missing_value(tmp): | ||
| # test21: an options branch missing value: → ValueError. | ||
| argv = ['generate', 'test21', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| with raises(ValueError, match="missing required key: value"): | ||
| app.run() | ||
|
|
||
|
|
||
| def test_generate_features_top_level_silent_variable(tmp): | ||
| # test22: top-level variable with `prompt: false` — silent | ||
| # sentinel works outside select-mode features too. The default | ||
| # lands verbatim in the rendered output. | ||
| argv = ['generate', 'test22', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| app.run() | ||
|
|
||
| assert exists_join(tmp.dir, 'take-me-silent') | ||
| with open(os.path.join(tmp.dir, 'take-me-silent')) as f: | ||
| res = f.read() | ||
| assert 'myapp' in res | ||
| assert 'top_level_silent_value' in res | ||
|
|
||
|
|
||
| def test_generate_features_legacy_null_variables(tmp): | ||
| # test23: regression — legacy boolean feature with | ||
| # `enabled: { variables: null, exclude: null, ignore: null }` | ||
| # must coalesce safely via the `block.get(...) or []` form on | ||
| # the legacy merge path. | ||
| argv = ['generate', 'test23', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| app.run() | ||
|
|
||
| assert exists_join(tmp.dir, 'take-me') | ||
|
|
||
|
|
||
| def test_generate_features_select_null_variables(tmp): | ||
| # test24: regression — select-mode options branch with | ||
| # `variables: null, exclude: null, ignore: null` must coalesce | ||
| # safely via the same `block.get(...) or []` form on the select | ||
| # merge path. | ||
| argv = ['generate', 'test24', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| app.run() | ||
|
|
||
| assert exists_join(tmp.dir, 'take-me') | ||
|
|
||
|
|
||
| def test_generate_features_select_requires_cascade(tmp): | ||
| # test25: select-mode feature has `requires: [bool_prereq]` where | ||
| # bool_prereq defaults to false. The cascade returns False for | ||
| # the select feature, the merge falls into the legacy | ||
| # `state is bool` branch with no `disabled:` block (not allowed | ||
| # in select mode), and the options-branch ignore patterns DO NOT | ||
| # fire — `should-not-appear-1` remains in the rendered tree. | ||
| argv = ['generate', 'test25', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| app.run() | ||
|
|
||
| assert exists_join(tmp.dir, 'take-me') | ||
| assert exists_join(tmp.dir, 'should-not-appear-1') | ||
|
|
||
|
|
||
| def test_generate_features_select_empty_options(tmp): | ||
| # test26: prompt_mode: select with `options: []` → ValueError. | ||
| argv = ['generate', 'test26', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| with raises(ValueError, match="no 'options' branches"): | ||
| app.run() | ||
|
|
||
|
|
||
| def test_generate_features_silent_variable_no_default(tmp): | ||
| # test27: top-level `prompt: false` variable with no `default:` → | ||
| # AssertionError from the silent-variable short-circuit. | ||
| argv = ['generate', 'test27', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| with raises(AssertionError, | ||
| match="prompt: false but no default"): | ||
| app.run() | ||
|
|
||
|
|
||
| def test_generate_features_select_missing_default(tmp): | ||
| # test28: prompt_mode: select feature with no `default:` → | ||
| # ValueError. Without `default`, --defaults dispatch would | ||
| # silently no-op (str(None) matches no option); fail-fast at | ||
| # validation per CONTEXT.md Decision F. | ||
| argv = ['generate', 'test28', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| with raises(ValueError, match="has no 'default' value"): | ||
| app.run() | ||
|
|
||
|
|
||
| def test_generate_features_silent_variable_non_str_default(tmp): | ||
| # test29: silent variable with a YAML-decoded non-string default | ||
| # (`default: false` → Python bool) + `case: upper`. The silent | ||
| # short-circuit must coerce to str BEFORE the case operation runs; | ||
| # otherwise `False.upper()` raises AttributeError. | ||
| argv = ['generate', 'test29', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| app.run() | ||
|
|
||
| assert exists_join(tmp.dir, 'take-me') | ||
| with open(os.path.join(tmp.dir, 'take-me')) as f: | ||
| # `case: upper` applied to str(False) → "FALSE". | ||
| assert 'silent_flag=FALSE' in f.read() | ||
|
|
||
|
|
||
| def test_generate_features_select_duplicate_labels(tmp): | ||
| # test30: two options with the same effective display label | ||
| # ("Same Label") → ValueError. The numbered list would be | ||
| # ambiguous so we reject at config-validation time. | ||
| argv = ['generate', 'test30', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| with raises(ValueError, match="duplicate option labels"): | ||
| app.run() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type annotations to all new test functions.
All 15 new test functions are missing type annotations for the tmp parameter and return type. As per coding guidelines, Python code must use type annotations for all function parameters and return types.
📝 Recommended fix: Add type annotations
Example for the first function (apply the same pattern to all 15 new test functions):
-def test_generate_features_select_defaults(tmp):
+def test_generate_features_select_defaults(tmp: Any) -> None:
# test16: prompt_mode: select with three options; --defaultsApply to all new test functions:
test_generate_features_select_defaultstest_generate_features_select_collisiontest_generate_features_select_invalid_prompt_modetest_generate_features_select_silent_variabletest_generate_features_select_default_not_in_valuestest_generate_features_select_missing_valuetest_generate_features_top_level_silent_variabletest_generate_features_legacy_null_variablestest_generate_features_select_null_variablestest_generate_features_select_requires_cascadetest_generate_features_select_empty_optionstest_generate_features_silent_variable_no_defaulttest_generate_features_select_missing_defaulttest_generate_features_silent_variable_non_str_defaulttest_generate_features_select_duplicate_labels
Add Any to the imports at the top of the file:
import os
import re
from unittest.mock import patch
+from typing import Any
from cement import Controller, TestAppAs per coding guidelines: "Use type annotations for all function parameters and return types in Python code"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/ext/test_ext_generate.py` around lines 372 - 556, All listed test
functions (e.g., test_generate_features_select_defaults,
test_generate_features_select_collision, ...,
test_generate_features_select_duplicate_labels) need type annotations; add a
parameter annotation for tmp (tmp: Any) and a return annotation -> None on each
function, and add "from typing import Any" to the file imports so the symbol Any
is available.
Member
Author
|
merged to main. |
Contributor
|
Hi @derks its working :-) But let me point out some additional:
but not:
and not:
does not work: instead of e.g.: for: Looking forward to your feedback |
Member
Author
|
Thanks for the detailed feedback, @TomFreudenberg! 🙇 Since this PR is now closed, I've captured your comment in a new issue to track these follow-ups: #782 (milestone 3.0.16 Stable). Let's continue the discussion there. |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends
cement/ext/ext_generate.py(the.generate.ymltemplateengine) so a single feature can support more than two answers. Today
a feature prompt is boolean-like —
Enable Feature: X [Y/n]— anddispatches into either an
enabled:ordisabled:config block. ThisPR adds an opt-in
prompt_mode: selectthat presents a numberedpicker and dispatches the chosen value into one of N named branches
under
options:. Resolves #779.Implementation delegates directly to
cement.utils.shell.Promptrather than building parallel prompt/validation infrastructure —
shell.Prompt.Meta.options+numbered=True+case_insensitive=TrueMeta.default+ the built-inmax_attemptsretry loop cover thewhitelist, display, matching, and fallback behavior end-to-end.
The legacy boolean path is byte-identical when
prompt_modeisabsent or set to
boolean(the explicit default name). Everypre-existing test fixture (
tests/data/templates/generate/test1–test15) and the existing demo template walk through the new codeunchanged.
What landed
Schema — opt-in multi-valued prompts (
feat:commit)prompt_mode,default(required forselect-mode),
options.options:is a list of dicts each with a requiredvalue:keyand an optional
prompt:label (falls back tostr(value)).Labels are displayed in
shell.Prompt's numbered list;value:is what flows downstream as the resolved feature state
(
{{ web_framework }}substitutes this string).optionsbranch may declareignore/exclude/variables— same vocabulary as the existingenabled:/disabled:blocks.prompt: falsesentinel on avariable definition — the value lands from
defaultwithoutrendering an interactive prompt. Works inside
optionsbranchesand at the top level.
caseandvalidatestill apply to thesilent default;
str()coerce so YAML-decoded non-string defaults(bool, int) don't crash the case/validate operations.
requiresvalidator discipline):
ValueErroron schema collision(
prompt_mode: select+enabled/disabledblocks), unknownprompt_mode, missingvalue:key, emptyoptions:, missingdefault:, default not inoptionsvalues, duplicate optionlabels.
or []null-safety idiom (per commit 6895aa5) isgeneralized to the per-block keys on both the legacy and
select-mode merge paths for consistency.
Demo — living example of both modes (
docs(demo):commit)demo/generate-features/extended additively with a newweb_frameworkselect-mode feature (None / Flask / FastAPI) thatuses silent
framework_versionvariables under theflask/fastapibranches andignorepatterns on thenonebranch tosuppress two new template files (
requirements.txt,wsgi.py).docker/docker_composeboolean features staybyte-identical — they just gain an explicit
prompt_mode: booleanfor clarity. Omission and the explicit form are exactly
equivalent (backward-compat absolute).
section walking through all the new keys, the numbered picker UX,
silent variables, the
--defaultsheadless path, and therequires:cascade interaction.Changelog (
docs(changelog):commit)One line under the active
## 3.0.15 - DEVELOPMENT/ Featuresbucket per the project's CHANGELOG discipline (
[ext.generate]area tag).
Picker UX
A sequence of feature prompts renders as:
The select-mode prompt's leading blank line and trailing
:keepit consistent with the boolean feature prompt and variable prompts
(both of which hardcode
:at the end of their format strings).User picks
2,shell.Promptreturns"Flask Web Framework", themerge step maps that label back to
value: "flask"via positionalzip, walks the
options[1]branch, and applies itsvariables/ignore.Acceptance status
prompt_mode: selectexplicit opt-in; legacy path byte-identical when absent orprompt_mode: booleanoptions:is list of dicts with requiredvalue:and optionalprompt:labelprompt: false; case+validate still apply to default; str-coerce for non-string YAML defaultsValueErroron schema collision / unknown prompt_mode / missing keys / default not in values / duplicate labelsdefault:is REQUIRED for select-mode featuresprompt_modedefaults to'boolean'explicitly; whitelist validator rejects everything elsecement.utils.shell.Prompt(no parallel infrastructure)Files touched
cement/ext/ext_generate.py_process_featuresextended with select-mode validator +_resolvemode dispatch + select-mode mergetests/ext/test_ext_generate.pytests/data/templates/generate/test16–test30.generate.ymlfixture directoriesdemo/generate-features/{README.md, templates/generate/webapp/.generate.yml, requirements.txt, wsgi.py}web_frameworkfeature + README walkthrough; explicitprompt_mode: booleanon existing featuresCHANGELOG.md3.0.15 - DEVELOPMENT.planning/STATE.mdTest plan
make comply— ruff + mypy greenpdm run pytest --cov=cement.ext.ext_generate --cov-fail-under=100 tests/ext/test_ext_generate.py— 100% coverage on the extension (202/202 stmts), 35 tests passmake audit-public-api— empty diff vs Phase 03 baseline (no public API leaked)pdm run pytest tests/ext/test_ext_generate.py— all 35 tests pass (10 pre-existing memcached/redis failures elsewhere are infra, not this PR)cd demo/generate-features && pdm run python myapp.py generate webapp /tmp/myproject --defaults --force— renders withweb_framework=nonedefaults (norequirements.txt, nowsgi.pyin output)cd demo/generate-features && pdm run python myapp.py generate webapp /tmp/myprojectand pick3at the Web Framework prompt — renders withwsgi.pyand arequirements.txtcontainingfastapi==0.115tests/data/templates/generate/test12) walks the new code path byte-identically (legacy boolean regression anchor)Closes #779.
Summary by CodeRabbit
Release Notes
New Features
prompt_mode: selectfor feature prompts, enabling users to choose from multiple options with per-option configuration (variables, ignore patterns).prompt: false) that uses defaults without prompting.Documentation