feat(ext.generate): unify features into typed variables (#782)#783
Open
derks wants to merge 14 commits into
Open
feat(ext.generate): unify features into typed variables (#782)#783derks wants to merge 14 commits into
derks wants to merge 14 commits into
Conversation
- Migrate test6/test7/test13 fixtures from features: to variables: with type: boolean + extend: (enabled:->when:true, disabled:->when:false) - Add test31 string-form bool-prompt fixture + patched-prompt unit tests covering y/yes->True, n/no->False, empty->default mapping - Add test32 D-17 fixture + test: type:boolean declaring case: -> ValueError - Tests fail until the resolver lands in Task 2 (expected RED)
- Add single recursive resolve_and_emit walking variables: in declaration order; every variable emits at top-level data[name] (delete the data['features'] namespace — #782 bug-4 fix) - Add type: key (string|boolean|choice, default string); type: string is byte-identical to the released schema (test1-5/22/27/29 oracle) - type: boolean emits a real Python bool so {% if feature_x %} works; string-form prompt '[(Y)es/(N)o] [default]:' with y/yes->True, n/no->False, empty->default mapping (covered via patched-prompt test) - D-17 guard: type: boolean/choice declaring case:/validate: -> ValueError - Unknown type: -> ValueError (T-05.1-01 input-validation mitigation) - extend: boolean when:true/false accumulates variables/exclude/ignore and recurses nested vars depth-first in place - Keep a thin legacy features: bridge folding enabled/disabled/options into the unified accumulation so un-migrated test8-30 stay green; bridge removed in Plan 03 (user-approved Rule 4 branch-by-abstraction) - Add test33 (silent bool) + test34 (invalid type) for 100% coverage
- migrate test16/19/20/21/26/28/30 from prompt_mode:select to type:
choice + options: + extend: (when-keyed effects)
- add test35 object-form bool prompt {text, accept, reject} fixture
- add test36 unquoted bool-like accept member (YAML 1.1 coercion)
- add test37 type:choice declaring case: (D-17 string-only) fixture
- rewrite select tests for choice schema; add picker/accept/reject/
junk-abort/coercion-guard/D-17-choice tests (RED before Task 2)
- type: choice numbered picker emits the chosen option str at
data[name]; fail-fast ValueError on empty options / option missing
value / default-not-in-values / duplicate labels (D-16, Q3)
- polymorphic boolean prompt object-form {text, accept, reject}: author
owns the text; accept/reject case-insensitive token lists map to bool;
junk input asserts + aborts 'Invalid Response' (D-12/D-14)
- YAML 1.1 bool-coercion load guard: a bool-coerced accept/reject member
raises ValueError telling the author to quote it (Pitfall 1, T-05.1-04)
- extend.when scalar str()-coerced equality for choice/string; booleans
still match by Python == (Q2) — test16/test19 fire their per-option
rules at this commit
- D-17 case:/validate: on type:choice -> ValueError (string-only)
- pragma the now-dead legacy select-validation raises (owned by the new
resolver under real coverage; bridge removed in Plan 03)
- migrate test8-12/14/15/23/24/25 to type:/extend:/requires: schema - remove obsolete test17/test18 (prompt_mode/enabled/disabled gone; type: whitelist already covered by test34) - add test38 (extend.when list membership), test39 (string regex), test40 (Q1 requires-gated-out renders default) - adapt assertions to new semantics: gated-out var renders its default and omits its extend effects (Q1); requires unknown -> ValueError - RED until the resolver lands list/regex/nesting/requires in Task 2
- generalize _when_matches to compose scalar/in-list/string-regex
forms; multiple matching extend rules compose and accumulate
- nested extend.variables resolve depth-first in place only when the
parent rule fires (D-08)
- add top-level requires: with full match vocab ([name] truthy,
{name: value} equality, {name: [v]} in-list), AND-ed and resolved
order-independently via lazy memoized recursion (D-09/D-11)
- a requires-gated-out var renders as its default (Q1) and its extend
rules do not fire; unknown requires target -> ValueError
- DELETE the legacy features:/_process_features bridge; the engine no
longer reads a top-level features: key (GEN-01 fully true)
- variable missing name -> ValueError (was assert)
- test38/41 cover boolean list-form when + dict-form requires equality
Resolves #782
- Rewrite webapp .generate.yml under variables: with type:/extend:/requires: (docker/docker_compose boolean, web_framework choice; drop features:) - docker_compose requires: [docker]; per-branch extend.when ignore + silent framework_version; docker extend.when true prompts python_version - Demonstrate #782 fix: app.py/README use {% if docker %} and {% if web_framework == ... %} against top-level bool/choice exposure - Rewrite demo README to type:/extend:/requires: vocabulary, document the {{ bool }} -> True/False interpolation gotcha
- Remove the four bare # pragma: nocover markers on the type: choice picker setup (default_label / prompt_text / SelectPrompt class) in _resolve_choice — they lacked a locked-vocabulary category label and excluded code that the patched-prompt test actually exercises. - test_generate_choice_picker_maps_label_to_value drives the picker end to end, so the bug-prone label->value mapping is now under real coverage (269 stmts, 0 miss) rather than pragma'd. - Only the raw SelectPrompt().prompt() input read stays excluded, with its existing # defensive: unreachable label. - Reword the dynamic-import FIXME so it no longer contains a literal 'pragma: nocover' token that tripped the inverse audit grep.
- Reference PR #780 (Tom Freudenberg's review feedback) alongside #782 on the unified type:/extend:/requires: Features entry, calling out the four addressed points: features after vars in declaration order, custom per-feature prompt text, vars-style input, top-level Jinja boolean exposure. - Rewrap the demo-migration [dev] entry to keep every line <=78 chars.
A type: boolean variable with no prompt: key rendered the literal
"None [(Y)es/(N)o] [<hint>]:" because var['prompt'] was None. Default
the label to f"Enable {name}" when prompt is falsy (D-12), mirroring
the choice path's `or f"Select: {name}"` fallback. An explicit
prompt: still overrides; the object-form is unaffected.
Covered by test42 (boolean, no prompt:) via a recorder patched onto
shell.Prompt.prompt that captures the bound Meta.text and asserts the
"Enable enable_thing [(Y)es/(N)o]" label plus the explicit-override
case.
Correct the demo README: requires: uses a bare-name list (truthy) or a
{name: value} map (equality/in-list/regex) — not the list-of-objects
form the engine rejects. Document the boolean default label
(Enable <name>) and the object-form prompt (text/accept/reject) with
the YAML bool-coercion quoting note.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR replaces Cement's legacy ChangesCore Typed Variable System
Documentation and Demo Migration
Test Fixture Migration
Test Suite Expansion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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: 13
🤖 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 `@cement/ext/ext_generate.py`:
- Around line 308-328: In _when_matches, don't treat every string when as a
regex; change the regex branch to only run when when is an actual compiled regex
(e.g., isinstance(when, re.Pattern)) or when an explicit regex marker is used,
and otherwise fall back to exact string equality for vtype == 'string' (so
literal when values like "flask" match exactly and values with regex
metacharacters are not interpreted). Update the regex check in the _when_matches
function and ensure the final fallback uses str(when) == str(value) for string
scalars.
- Around line 340-375: Add cycle detection for recursive variable resolution by
introducing a "resolving" set and using it in resolve_and_emit to detect
re-entrance: when resolve_and_emit(var) starts, if var['name'] is already in
resolving raise a ValueError describing the cycle (include the var name and
ideally the current stack of resolving names); otherwise add the name to
resolving, proceed with resolution, memoize into data as before, and finally
remove the name from resolving in a finally block. Keep _requires_satisfied, and
other call sites (the code that calls resolve_and_emit in the other blocks
referenced) unchanged except they will now get a clear ValueError instead of
RecursionError; reference resolve_and_emit, _requires_satisfied, var_by_name,
data and ensure resolving is accessible to both resolve_and_emit and the
callers' scope.
- Around line 330-338: _gated_default currently stringifies None for gated-out
string/choice vars; change _gated_default(var, vtype) to first verify that var
contains a real default (e.g. 'default' in var and var['default'] is not None)
and if not, raise a validation error (or otherwise reject) instead of returning
"None"; keep the boolean branch casting to bool, and for choice/string types
ensure the default passes the same validation used by _resolve_choice (reuse or
call that validation) before returning str(default). Apply the same guard to the
duplicate implementation around the other occurrence (the block at the later
406-412 location).
In `@CHANGELOG.md`:
- Around line 225-230: The CHANGELOG entry for `[ext.generate]` spans multiple
lines; collapse it to a single-line entry prefixed with `[ext.generate]` that
summarizes the change succinctly (mention removal of the unreleased `features:`
schema and deletion of the legacy compatibility bridge, and reference issue `#782`
if needed) so the entire note fits on one line in CHANGELOG.md.
- Around line 190-196: The CHANGELOG entry for [ext.generate] describing the
`type: choice` template variables spans multiple lines; collapse it into a
single-line entry prefixed with `[ext.generate]` that summarizes the feature
(mentioning `type: choice`, `cement.utils.shell.Prompt`, and key behaviors like
`options:` format and fail-fast validation) so it follows the one-line-per-entry
guideline.
- Around line 160-165: The CHANGELOG entry for [ext.generate] spans multiple
lines; collapse it into a single-line entry prefixed with [ext.generate] that
briefly states the change (e.g. "type: boolean template variables now render as
Python bools so jinja2/mustache conditionals work; boolean prompt format and
single-pass prompting; resolves `#782`") and move the detailed explanation about
`data[name]`, `{% if feature_x %}`, the `[(Y)es/(N)o] [default]:` prompt format,
and the removal of the former `features:` namespace/pre-pass into the associated
PR/issue description.
- Around line 197-205: Consolidate the multi-line CHANGELOG entry for
[ext.generate] into a single line: rewrite the paragraph as one line starting
with [ext.generate] and briefly state that `type: boolean` `prompt:` is now
polymorphic — supports framework string, `prompt: false`, or an object form
`{text, accept, reject}` where case-insensitive `accept`/`reject` tokens map
input to a bool (unmatched input errors as `Invalid Response` mirroring
`validate:`) and YAML 1.1 bool-like `accept`/`reject` tokens raise `ValueError`
instructing authors to quote them; ensure no line breaks so the entire entry is
one line.
- Around line 232-236: Consolidate the multi-line CHANGELOG entry into a
single-line entry prefixed with [dev]; replace the 5-line block mentioning
`demo/generate-features/`, the migration from `features:` to
`type:`/`extend:`/`requires:`, and the `#782` rendering note into one concise line
(e.g. "[dev] Migrate demo/generate-features/ template to unified
type/extend/requires schema; demonstrate `#782` fix where `{% if docker %}` and
`{% if web_framework == ... %}` now render against top-level bool/choice
exposure").
- Around line 206-221: The changelog entry for the `[ext.generate]` feature is
currently multi-line; collapse it into a single-line entry starting with the
`[ext.generate]` prefix that summarizes the change (mention compose three match
forms, requires gating, and PR/Issue references) and keep the links `(Issue
`#782`, PR `#780`)` inline; ensure the line is one continuous line (no line breaks)
and follows the "Each CHANGELOG.md entry must be one line, prefixed with [area]"
rule so the existing multi-line block is replaced with that single consolidated
line.
In `@demo/generate-features/README.md`:
- Line 272: Remove the unused Markdown link reference "[779]" by deleting the
line that defines "[779]: https://github.com/datafolklabs/cement/issues/779"
(ensure no other references to [779] exist elsewhere in the document); this
cleans up the README and avoids an orphaned link definition now superseded by
issue `#782`.
- Around line 157-160: The blockquote containing the text starting with "Quote
bool-like tokens (`\"yes\"`, `\"no\"`, `\"on\"`, `\"off\"`) inside `accept:` /
`reject:` — under YAML 1.1 they otherwise decode to a Python `bool` and the
loader rejects them with a clear error." contains an extra blank line that
violates MD028; remove the blank line so the two quoted lines are merged into a
single blockquote paragraph, or alternatively split them into two separate
blockquote blocks so there is no empty line inside a single blockquote.
In `@tests/ext/test_ext_generate.py`:
- Around line 365-374: The test test_generate_boolean_silent is not asserting
the actual typed output and can pass if the boolean is wrongly stringified;
change the test to verify the raw rendered value or flip the default to false so
string 'True' fails — specifically update test_generate_boolean_silent (and/or
the fixture used by GenerateApp) to assert the file contents equal the expected
literal boolean representation (or use a false default so only a real Python
True/False yields the expected branch), e.g., open the generated 'take-me' file
after app.run() and assert the exact value/type rather than only that 'flag-on'
appears.
- Around line 294-305: The new tests (e.g., test_generate_boolean_prompt_yes)
and related helpers like record() lack Python type annotations; update each test
function signature to add parameter and return type annotations (e.g., annotate
the tmp parameter type and the function return as -> None) and annotate the
record() helper's parameters and return type so all new/changed tests comply
with the project's typing rule; ensure every test and helper introduced in this
diff uses explicit type hints for parameters and return values.
🪄 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: f13485d2-bbc2-4666-9aee-0416b870d66a
📒 Files selected for processing (64)
CHANGELOG.mdcement/ext/ext_generate.pydemo/generate-features/README.mddemo/generate-features/templates/generate/webapp/.generate.ymldemo/generate-features/templates/generate/webapp/README.mddemo/generate-features/templates/generate/webapp/app.pytests/data/templates/generate/test10/.generate.ymltests/data/templates/generate/test11/.generate.ymltests/data/templates/generate/test12/.generate.ymltests/data/templates/generate/test13/.generate.ymltests/data/templates/generate/test14/.generate.ymltests/data/templates/generate/test14/take-metests/data/templates/generate/test15/.generate.ymltests/data/templates/generate/test16/.generate.ymltests/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/test20/.generate.ymltests/data/templates/generate/test21/.generate.ymltests/data/templates/generate/test23/.generate.ymltests/data/templates/generate/test24/.generate.ymltests/data/templates/generate/test25/.generate.ymltests/data/templates/generate/test26/.generate.ymltests/data/templates/generate/test28/.generate.ymltests/data/templates/generate/test30/.generate.ymltests/data/templates/generate/test31/.generate.ymltests/data/templates/generate/test31/take-metests/data/templates/generate/test32/.generate.ymltests/data/templates/generate/test32/take-metests/data/templates/generate/test33/.generate.ymltests/data/templates/generate/test33/take-metests/data/templates/generate/test34/.generate.ymltests/data/templates/generate/test34/take-metests/data/templates/generate/test35/.generate.ymltests/data/templates/generate/test35/take-metests/data/templates/generate/test36/.generate.ymltests/data/templates/generate/test36/take-metests/data/templates/generate/test37/.generate.ymltests/data/templates/generate/test37/take-metests/data/templates/generate/test38/.generate.ymltests/data/templates/generate/test38/bool-list-ignoredtests/data/templates/generate/test38/fastapi-onlytests/data/templates/generate/test38/not-webtests/data/templates/generate/test38/take-metests/data/templates/generate/test39/.generate.ymltests/data/templates/generate/test39/not-v2tests/data/templates/generate/test39/take-metests/data/templates/generate/test39/v3-onlytests/data/templates/generate/test40/.generate.ymltests/data/templates/generate/test40/should-be-kepttests/data/templates/generate/test40/take-metests/data/templates/generate/test41/.generate.ymltests/data/templates/generate/test41/dep-no-kepttests/data/templates/generate/test41/dep-ok-ignoredtests/data/templates/generate/test41/take-metests/data/templates/generate/test42/.generate.ymltests/data/templates/generate/test42/take-metests/data/templates/generate/test6/.generate.ymltests/data/templates/generate/test7/.generate.ymltests/data/templates/generate/test8/.generate.ymltests/data/templates/generate/test9/.generate.ymltests/ext/test_ext_generate.py
💤 Files with no reviewable changes (4)
- tests/data/templates/generate/test18/take-me
- tests/data/templates/generate/test18/.generate.yml
- tests/data/templates/generate/test17/take-me
- tests/data/templates/generate/test17/.generate.yml
Comment on lines
+308
to
+328
| def _when_matches(value: Any, when: Any, vtype: str) -> bool: | ||
| # Compose the three `extend.when` / `requires` match forms | ||
| # (D-07), dispatching on the carrying variable's `type` (Q2): | ||
| # - list -> in-list membership. Booleans compare by Python | ||
| # `==`; choice/string members str()-coerce both sides. | ||
| # - regex -> `re.match(when, value)` — STRING type only | ||
| # (mirrors `validate:`); a non-string `when` on a string | ||
| # var still falls through to scalar equality below. | ||
| # - scalar -> equality. Booleans by Python `==` (so | ||
| # `when: true` matches `True`, not the string "True"); | ||
| # choice/string by str()-coerced equality (so `when: 1` | ||
| # matches a "1" option value; per the L219 discipline). | ||
| if isinstance(when, list): | ||
| if vtype == 'boolean': | ||
| return value in when | ||
| return str(value) in [str(w) for w in when] | ||
| if vtype == 'string' and isinstance(when, str): | ||
| return re.match(when, value) is not None | ||
| if vtype == 'boolean': | ||
| return bool(value == when) | ||
| return str(when) == str(value) |
There was a problem hiding this comment.
Don't treat every string when as a regex.
Line 324 makes scalar string rules impossible: when: flask also matches flask2, and literals containing regex metacharacters are interpreted unexpectedly. That breaks the advertised equality semantics for string variables in both requires and extend; this needs an explicit regex form or a separate exact-match path.
🤖 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 308 - 328, In _when_matches, don't
treat every string when as a regex; change the regex branch to only run when
when is an actual compiled regex (e.g., isinstance(when, re.Pattern)) or when an
explicit regex marker is used, and otherwise fall back to exact string equality
for vtype == 'string' (so literal when values like "flask" match exactly and
values with regex metacharacters are not interpreted). Update the regex check in
the _when_matches function and ensure the final fallback uses str(when) ==
str(value) for string scalars.
Comment on lines
+330
to
+338
| def _gated_default(var: dict[str, Any], vtype: str) -> Any: | ||
| # Value emitted for a `requires`-gated-out variable (Q1): its | ||
| # own `default`, typed like a fully-resolved value, so the | ||
| # template never KeyErrors. boolean -> Python bool; | ||
| # choice/string -> str(default) (matching the resolved-path | ||
| # str invariant). A gated-out var's extend rules do NOT fire. | ||
| if vtype == 'boolean': | ||
| return bool(var['default']) | ||
| return str(var['default']) |
There was a problem hiding this comment.
Reject gated variables that don't have a real default.
When requires fails, _gated_default() stringifies None, so a gated-out string or choice variable lands in the template context as the literal "None". The choice path also bypasses _resolve_choice()'s required-default validation here.
Suggested guard
def _gated_default(var: dict[str, Any], vtype: str) -> Any:
# Value emitted for a `requires`-gated-out variable (Q1): its
# own `default`, typed like a fully-resolved value, so the
# template never KeyErrors. boolean -> Python bool;
# choice/string -> str(default) (matching the resolved-path
# str invariant). A gated-out var's extend rules do NOT fire.
+ if var['default'] is None:
+ raise ValueError(
+ f"Variable '{var['name']}' uses requires: but has no default")
if vtype == 'boolean':
return bool(var['default'])
return str(var['default'])Also applies to: 406-412
🤖 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 330 - 338, _gated_default currently
stringifies None for gated-out string/choice vars; change _gated_default(var,
vtype) to first verify that var contains a real default (e.g. 'default' in var
and var['default'] is not None) and if not, raise a validation error (or
otherwise reject) instead of returning "None"; keep the boolean branch casting
to bool, and for choice/string types ensure the default passes the same
validation used by _resolve_choice (reuse or call that validation) before
returning str(default). Apply the same guard to the duplicate implementation
around the other occurrence (the block at the later 406-412 location).
Comment on lines
+340
to
+375
| def _requires_satisfied(var: dict[str, Any]) -> bool: | ||
| # Evaluate top-level `requires:` (D-09/D-11), AND-ed across | ||
| # keys. Each referenced variable is resolved ON DEMAND (lazy | ||
| # recursion + memoize-on-`data`), so a prerequisite declared | ||
| # LATER still resolves first — resolution is order-independent. | ||
| # Vocab forms (matching `extend.when`): | ||
| # - `requires: [name]` -> `name` must be truthy | ||
| # - `requires: {name: value}` -> equality / regex / in-list | ||
| # via `_when_matches` | ||
| requires = var['requires'] | ||
| if requires is None: | ||
| return True | ||
|
|
||
| if isinstance(requires, dict): | ||
| items = list(requires.items()) | ||
| else: | ||
| # list/sugar form: each entry is a bare name -> truthy | ||
| items = [(name, None) for name in requires] | ||
|
|
||
| for req_name, expected in items: | ||
| if req_name not in var_by_name: | ||
| raise ValueError( | ||
| f"Variable '{var['name']}' requires unknown " | ||
| f"variable '{req_name}'") | ||
| resolve_and_emit(var_by_name[req_name]) | ||
| req_value = data[req_name] | ||
| req_var = var_by_name[req_name] | ||
| if expected is None: | ||
| # truthy sugar | ||
| if not req_value: | ||
| return False | ||
| else: | ||
| if not _when_matches(req_value, expected, | ||
| req_var.get('type', 'string')): | ||
| return False | ||
| return True |
There was a problem hiding this comment.
Add cycle detection around recursive resolution.
resolve_and_emit() only memoizes after a variable finishes resolving. A cycle like A requires B and B requires A will recurse until Python raises RecursionError, and the same problem applies to nested extend.variables graphs.
Suggested guard
+ resolving = set()
+
def resolve_and_emit(defined_var: dict[str, Any]) -> None:
var = var_defaults.copy()
var.update(defined_var)
if var['name'] is None:
raise ValueError(
"Required generate config key missing: name")
# Memoize on `data` membership so a variable referenced more than
# once (e.g. via `requires`) resolves exactly once and resolution
# stays declaration-order-independent (D-11).
if var['name'] in data:
return
+ if var['name'] in resolving:
+ raise ValueError(
+ f"Cyclic variable dependency detected for '{var['name']}'")
+
+ resolving.add(var['name'])
+ try:
+ vtype = var['type']
+ if vtype not in ('string', 'boolean', 'choice'):
+ raise ValueError(
+ f"Variable '{var['name']}' has invalid type "
+ f"'{vtype}' (must be string, boolean, or choice)")
- vtype = var['type']
- if vtype not in ('string', 'boolean', 'choice'):
- raise ValueError(
- f"Variable '{var['name']}' has invalid type "
- f"'{vtype}' (must be string, boolean, or choice)")
-
- # D-17: case:/validate: are string-only semantics. A boolean is
- # parsed via accept/reject; a choice is constrained by options —
- # declaring case:/validate: on either is a fail-fast schema
- # misconfig (ValueError survives `python -O`).
- if vtype in ('boolean', 'choice') and \
- (var['case'] is not None or var['validate'] is not None):
- raise ValueError(
- f"case:/validate: are string-only; variable "
- f"'{var['name']}' is type: {vtype}")
+ # ... existing body unchanged ...
- if not _requires_satisfied(var):
- data[var['name']] = _gated_default(var, vtype)
- return
+ finally:
+ resolving.remove(var['name'])Also applies to: 377-388, 434-435
🤖 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 340 - 375, Add cycle detection for
recursive variable resolution by introducing a "resolving" set and using it in
resolve_and_emit to detect re-entrance: when resolve_and_emit(var) starts, if
var['name'] is already in resolving raise a ValueError describing the cycle
(include the var name and ideally the current stack of resolving names);
otherwise add the name to resolving, proceed with resolution, memoize into data
as before, and finally remove the name from resolving in a finally block. Keep
_requires_satisfied, and other call sites (the code that calls resolve_and_emit
in the other blocks referenced) unchanged except they will now get a clear
ValueError instead of RecursionError; reference resolve_and_emit,
_requires_satisfied, var_by_name, data and ensure resolving is accessible to
both resolve_and_emit and the callers' scope.
Comment on lines
+160
to
+165
| - `[ext.generate]` `type: boolean` template variables now emit a real | ||
| Python bool at the top level of the render context (`data[name]`), | ||
| so `{% if feature_x %}` works in jinja2/mustache; the boolean prompt | ||
| uses a vars-style `[(Y)es/(N)o] [default]:` format and all variables | ||
| prompt in a single declaration-order pass — resolves #782 (the | ||
| former `features:` namespace and pre-pass are removed) |
There was a problem hiding this comment.
Consolidate to a single-line CHANGELOG entry.
CHANGELOG entries must be one line per the coding guidelines. This entry spans 6 lines. Condense the description or move detail to issue/PR discussions.
As per coding guidelines: "Each CHANGELOG.md entry must be one line, prefixed with [area]".
Example consolidation
-- `[ext.generate]` `type: boolean` template variables now emit a real
- Python bool at the top level of the render context (`data[name]`),
- so `{% if feature_x %}` works in jinja2/mustache; the boolean prompt
- uses a vars-style `[(Y)es/(N)o] [default]:` format and all variables
- prompt in a single declaration-order pass — resolves `#782` (the
- former `features:` namespace and pre-pass are removed)
+- `[ext.generate]` `type: boolean` variables emit real Python bools at top level for direct Jinja2/mustache conditionals; unified prompt pass — resolves `#782`📝 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
| - `[ext.generate]` `type: boolean` template variables now emit a real | |
| Python bool at the top level of the render context (`data[name]`), | |
| so `{% if feature_x %}` works in jinja2/mustache; the boolean prompt | |
| uses a vars-style `[(Y)es/(N)o] [default]:` format and all variables | |
| prompt in a single declaration-order pass — resolves #782 (the | |
| former `features:` namespace and pre-pass are removed) | |
| - `[ext.generate]` `type: boolean` variables emit real Python bools at top level for direct Jinja2/mustache conditionals; unified prompt pass — resolves `#782` |
🤖 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 160 - 165, The CHANGELOG entry for [ext.generate]
spans multiple lines; collapse it into a single-line entry prefixed with
[ext.generate] that briefly states the change (e.g. "type: boolean template
variables now render as Python bools so jinja2/mustache conditionals work;
boolean prompt format and single-pass prompting; resolves `#782`") and move the
detailed explanation about `data[name]`, `{% if feature_x %}`, the `[(Y)es/(N)o]
[default]:` prompt format, and the removal of the former `features:`
namespace/pre-pass into the associated PR/issue description.
Comment on lines
+190
to
+196
| - `[ext.generate]` Add `type: choice` template variables — a numbered | ||
| `cement.utils.shell.Prompt` picker that emits the chosen option string | ||
| at the top level of the render context. `options:` accepts a scalar | ||
| list or per-option `{value, prompt}` objects; per-option effects live | ||
| in `extend:` rules keyed by `when: <value>`. Misconfig (empty options, | ||
| option missing `value`, default not in options, duplicate labels) is | ||
| fail-fast `ValueError`. |
There was a problem hiding this comment.
Consolidate to a single-line CHANGELOG entry.
This feature entry spans 7 lines. Per coding guidelines, each CHANGELOG entry must be one line.
As per coding guidelines: "Each CHANGELOG.md entry must be one line, prefixed with [area]".
Example consolidation
-- `[ext.generate]` Add `type: choice` template variables — a numbered
- `cement.utils.shell.Prompt` picker that emits the chosen option string
- at the top level of the render context. `options:` accepts a scalar
- list or per-option `{value, prompt}` objects; per-option effects live
- in `extend:` rules keyed by `when: <value>`. Misconfig (empty options,
- option missing `value`, default not in options, duplicate labels) is
- fail-fast `ValueError`.
+- `[ext.generate]` Add `type: choice` variables with numbered picker; emits string at top level; `extend:` rules per option; fail-fast validation📝 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
| - `[ext.generate]` Add `type: choice` template variables — a numbered | |
| `cement.utils.shell.Prompt` picker that emits the chosen option string | |
| at the top level of the render context. `options:` accepts a scalar | |
| list or per-option `{value, prompt}` objects; per-option effects live | |
| in `extend:` rules keyed by `when: <value>`. Misconfig (empty options, | |
| option missing `value`, default not in options, duplicate labels) is | |
| fail-fast `ValueError`. | |
| - `[ext.generate]` Add `type: choice` variables with numbered picker; emits string at top level; `extend:` rules per option; fail-fast validation |
🤖 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 190 - 196, The CHANGELOG entry for [ext.generate]
describing the `type: choice` template variables spans multiple lines; collapse
it into a single-line entry prefixed with `[ext.generate]` that summarizes the
feature (mentioning `type: choice`, `cement.utils.shell.Prompt`, and key
behaviors like `options:` format and fail-fast validation) so it follows the
one-line-per-entry guideline.
Comment on lines
+232
to
+236
| - `[dev]` Migrate the `demo/generate-features/` webapp template to the | ||
| unified `type:`/`extend:`/`requires:` schema (off the removed | ||
| `features:` schema) and demonstrate the #782 fix — `{% if docker %}` | ||
| and `{% if web_framework == ... %}` now render against the top-level | ||
| bool/choice exposure |
There was a problem hiding this comment.
Consolidate to a single-line CHANGELOG entry.
This refactoring entry spans 5 lines. Per coding guidelines, each CHANGELOG entry must be one line.
As per coding guidelines: "Each CHANGELOG.md entry must be one line, prefixed with [area]".
Example consolidation
-- `[dev]` Migrate the `demo/generate-features/` webapp template to the
- unified `type:`/`extend:`/`requires:` schema (off the removed
- `features:` schema) and demonstrate the `#782` fix — `{% if docker %}`
- and `{% if web_framework == ... %}` now render against the top-level
- bool/choice exposure
+- `[dev]` Migrate `demo/generate-features/` to unified typed variable schema; demonstrates `#782` top-level bool/choice exposure📝 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
| - `[dev]` Migrate the `demo/generate-features/` webapp template to the | |
| unified `type:`/`extend:`/`requires:` schema (off the removed | |
| `features:` schema) and demonstrate the #782 fix — `{% if docker %}` | |
| and `{% if web_framework == ... %}` now render against the top-level | |
| bool/choice exposure | |
| - `[dev]` Migrate `demo/generate-features/` to unified typed variable schema; demonstrates `#782` top-level bool/choice exposure |
🤖 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 232 - 236, Consolidate the multi-line CHANGELOG
entry into a single-line entry prefixed with [dev]; replace the 5-line block
mentioning `demo/generate-features/`, the migration from `features:` to
`type:`/`extend:`/`requires:`, and the `#782` rendering note into one concise line
(e.g. "[dev] Migrate demo/generate-features/ template to unified
type/extend/requires schema; demonstrate `#782` fix where `{% if docker %}` and
`{% if web_framework == ... %}` now render against top-level bool/choice
exposure").
Comment on lines
+157
to
160
| > Quote bool-like tokens (`"yes"`, `"no"`, `"on"`, `"off"`) inside | ||
| > `accept:` / `reject:` — under YAML 1.1 they otherwise decode to a Python | ||
| > `bool` and the loader rejects them with a clear error. | ||
|
|
There was a problem hiding this comment.
Remove blank line inside blockquote.
The blank line at 160 inside the blockquote violates MD028. Merge the two blockquote paragraphs or separate them into distinct blocks.
📝 Proposed fix
> Quote bool-like tokens (`"yes"`, `"no"`, `"on"`, `"off"`) inside
> `accept:` / `reject:` — under YAML 1.1 they otherwise decode to a Python
> `bool` and the loader rejects them with a clear error.
-
+
> **`{{ bool }}` interpolation gotcha:** a boolean rendered as *text*🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 160-160: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 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 157 - 160, The blockquote
containing the text starting with "Quote bool-like tokens (`\"yes\"`, `\"no\"`,
`\"on\"`, `\"off\"`) inside `accept:` / `reject:` — under YAML 1.1 they
otherwise decode to a Python `bool` and the loader rejects them with a clear
error." contains an extra blank line that violates MD028; remove the blank line
so the two quoted lines are merged into a single blockquote paragraph, or
alternatively split them into two separate blockquote blocks so there is no
empty line inside a single blockquote.
| `requirements.txt` substitutes `fastapi==0.115` from the silent | ||
| `framework_version` variable on the fastapi branch. | ||
|
|
||
| [779]: https://github.com/datafolklabs/cement/issues/779 |
There was a problem hiding this comment.
Remove unused link reference.
The [779] link reference is defined but never used in the document. Issue #779 (select-mode features) was superseded by the unified #782 schema documented here.
🔗 Proposed fix
-[779]: https://github.com/datafolklabs/cement/issues/779
[782]: https://github.com/datafolklabs/cement/issues/782📝 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
| [779]: https://github.com/datafolklabs/cement/issues/779 | |
| [782]: https://github.com/datafolklabs/cement/issues/782 |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 272-272: Link and image reference definitions should be needed
Unused link or image reference definition: "779"
(MD053, link-image-reference-definitions)
🤖 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` at line 272, Remove the unused Markdown
link reference "[779]" by deleting the line that defines "[779]:
https://github.com/datafolklabs/cement/issues/779" (ensure no other references
to [779] exist elsewhere in the document); this cleans up the README and avoids
an orphaned link definition now superseded by issue `#782`.
Comment on lines
+294
to
+305
| def test_generate_boolean_prompt_yes(tmp): | ||
| # test31: type: boolean with a string-form prompt, run interactively | ||
| # (no --defaults). Patch shell.Prompt.prompt to return "yes" → the | ||
| # y/yes→True mapping runs and data[name] is True (jinja renders the | ||
| # enabled branch). | ||
| with patch.object(shell.Prompt, 'prompt', return_value='yes'): | ||
| argv = ['generate', 'test31', tmp.dir] | ||
| with GenerateApp(argv=argv) as app: | ||
| app.run() | ||
| with open(os.path.join(tmp.dir, 'take-me')) as f: | ||
| assert 'enabled' in f.read() | ||
|
|
There was a problem hiding this comment.
Add type annotations to the new tests and helper.
The new test cases start introducing unannotated tmp params and None returns, and the same issue repeats across the other added tests plus record(). That leaves the changed code out of compliance with the repo's Python typing rule.
Example pattern to apply
+from typing import Any
+
-def test_generate_boolean_prompt_yes(tmp):
+def test_generate_boolean_prompt_yes(tmp: Any) -> None:
...
- def record(self):
+ def record(self: shell.Prompt) -> str:
seen.append(self._meta.text)
return ''As 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 294 - 305, The new tests (e.g.,
test_generate_boolean_prompt_yes) and related helpers like record() lack Python
type annotations; update each test function signature to add parameter and
return type annotations (e.g., annotate the tmp parameter type and the function
return as -> None) and annotate the record() helper's parameters and return type
so all new/changed tests comply with the project's typing rule; ensure every
test and helper introduced in this diff uses explicit type hints for parameters
and return values.
Comment on lines
+365
to
+374
| def test_generate_boolean_silent(tmp): | ||
| # test33: type: boolean with `prompt: false` (silent) emits bool(default) | ||
| # at data[name] — a real Python bool, NOT str(default). jinja renders the | ||
| # enabled branch because the default is true. | ||
| argv = ['generate', 'test33', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| app.run() | ||
| with open(os.path.join(tmp.dir, 'take-me')) as f: | ||
| assert 'flag-on' in f.read() |
There was a problem hiding this comment.
This doesn't actually verify the silent path emits a bool.
With a truthy default, a broken implementation that returns 'True' instead of True still renders flag-on, so this test passes even when the typed-output contract regresses. Flip the fixture to a false default or assert the raw rendered value so stringification fails.
🤖 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 365 - 374, The test
test_generate_boolean_silent is not asserting the actual typed output and can
pass if the boolean is wrongly stringified; change the test to verify the raw
rendered value or flip the default to false so string 'True' fails —
specifically update test_generate_boolean_silent (and/or the fixture used by
GenerateApp) to assert the file contents equal the expected literal boolean
representation (or use a false default so only a real Python True/False yields
the expected branch), e.g., open the generated 'take-me' file after app.run()
and assert the exact value/type rather than only that 'flag-on' appears.
Contributor
|
Hi @derks that worked for me. I also tested with different "whens" and "requires". Looked complete by now. Let me please ask about your workflow, while many of your codes now generated by claude. Do you review all changes or is it mostly "YOLO" since 3.0.14? Have a nice sunday |
Member
Author
|
Hey @TomFreudenberg thanks for the feedback.
Right now I'm in a bit of an experimental phase, but have locked in at the moment to using GSD for many projects. Not sure if you've seen this issue/post yet; it outlines the current direction with AI (and the what/why):
Yes, I review all changes. Working with GSD through a phase looks like this: I won't say that I read everything - it's a lot. But I do review the plans before execution, otherwise you end up having to backout or redo a lot of the work. I review the code 100% ... that is where this one is at in the process. I'm treating the agent like I would working with a human ... set the direction, guide the work, let it make some of it's own informed decisions (research), and then submit the work for code review. Code Rabbit is here for a first pass "second set of eyes" - an alternative AI that is 100% outside the context of the planning/execution/etc. It reviews the outcome - same as if it were written by a human. My next pass will be to go through Code Rabbit's review with Claude, and discuss/implement changes based on the review. After that - I'll read though all of the code line by line and work through another session with Claude, maybe ask a lot of "why did we do X", and direct a lot of changes to clean it up. I might open my IDE and actually touch code - but to be honest it really disrupts the flow to modify files "out of band" ... changes might get reverted/stomped on by the agents if you don't remember to inform it to re-read files you touched. Once happy with the code - I'll squash/rebase down to a smaller set of commits and merge to |
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
Resolves #782 (Tom
Freudenberg's feedback on closed PR #780). The
generateextension's optionalfeatures mechanism is merged into the template variables mechanism so
that optional/conditional template inputs behave consistently with ordinary
variables — prompted in declaration order, exposed at the top level of the
template context, and with a sensible default-but-overridable prompt.
The entire
features:mechanism (boolean features and theprompt_mode: selectvariant) was unreleased — it first ships in 3.0.16 — so it is reshaped here
with no backward-compat shim. All changes to the released
variables:schema are strictly additive: a variable that declares none of the new keys
behaves byte-identically to today.
What an author writes now
A single
variables:list where each entry carries atype::Because every variable now lands at the top level of the render context
(
data[name]),{% if docker %}and{% if web_framework == "flask" %}workdirectly — the original Jinja bug in #782 (features were namespaced under
data['features']).Changes (by wave)
Wave 1 — unified typed resolver. Collapse
_process_featuresand the oldvariablesloop into one ordered, single-passresolve_and_emitwalker.Add
type: string|boolean|choice(defaultstring). Every variable emits attop-level
data[name]; thedata['features']namespace is removed. Booleanstring-form prompt renders
<prompt> [(Y)es/(N)o] [default]:, parsingy/yes→True,n/no→False, empty→default. D-17 guard:case:/validate:on a boolean/choice variable is a fail-fastValueError.Wave 2 — choice + polymorphic boolean prompt.
type: choicenumberedpicker (scalar list or
{value, prompt}option objects), emitting the chosenstring. Polymorphic boolean
prompt:— a plain string (framework owns thehint),
false(silent, usesdefault), or an object{text, accept, reject}where the author owns the text andaccept:/reject:are case-insensitive token lists (non-matching input aborts like a failed
validate:). A YAML 1.1 bool-coercion guard rejects unquotedyes/no/on/off/true/falsetokens inaccept:/reject:lists with aclear "quote it" error.
Wave 3 — conditional effects + dependencies; bridge removed.
extend.whencomposes three match forms — scalar equality, in-list membership, and (string
variables only) regex via
re.match— and multiple matching rules accumulatetheir
variables/exclude/ignore.extend.variablesnest depth-first,prompted in place only when their parent rule fires. Top-level
requires:gates a variable on others (bare-name list = truthy;
{name: value}map =equality/in-list/regex), AND-ed and resolved order-independently; a gated-out
variable takes its typed default so templates never error. The legacy
features:reader (~200 lines) is deleted —features:/prompt_mode:/enabled:/disabled:no longer exist in the engine.Wave 4 — demo migration.
demo/generate-features/rewritten to the unifiedschema and its README documents
type:/extend:/requires:, the polymorphicboolean prompt, and the
{{ bool }}capitalized-repr gotcha. All five CLIgeneratetemplates verified to still generate unchanged.Wave 5 — coverage audit + changelog. Removed redundant unlabeled
pragma: nocovermarkers that were masking already-tested mapping logic;every surviving pragma carries a locked-vocabulary label. CHANGELOG entries
for all four of #782's points.
Post-verification fix. Live interactive testing surfaced a defect the
automated suite missed: a boolean variable with no explicit
prompt:renderedits label as the literal
None. Fixed — the label now defaults toEnable <name>(e.g.Enable docker [(Y)es/(N)o] [Y]:); explicitprompt:still overrides. Covered by a new test.
Acceptance status
features:/prompt_mode:/enabled:/disabled:removed from the enginetype: string|boolean|choice; boolean→bool, choice→str, all top-level ({% if x %}works)[(Y)es/(N)o] [default]:boolean prompt; polymorphicprompt:(string/false/object)extend.whenvalue/list/regex (composing) + depth-first nesting + top-levelrequires:Files touched
cement/ext/ext_generate.py— unified typed resolver (+329 / −216).tests/ext/test_ext_generate.py+ ~30tests/data/templates/generate/test*/fixtures — migrated to the new schema; new tests for choice, accept/reject,
YAML guard,
extend.whenlist/regex, nesting,requires:, and thedefault-label fix.
demo/generate-features/— template, README, and webapp files migrated.CHANGELOG.md—[ext.generate]entries under the 3.0.16 developmentsection.
Test plan
make test— 370 passed, 100.00% coverage acrosscement/(requires Redis on 6379 + memcached on 11211 for the ext-cache tests).
make comply— ruff clean, mypy clean (51 source files).variables:oracle tests (test1–5/22/27/29) passunchanged.
generatetemplates produce output under--defaults.order with correct
Enable <name> [(Y)es/(N)o]labels, depth-first nesting,and
requires:gating.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
features:system with a typed variable system supportingtype: string,type: boolean, andtype: choice.requires:for variable gating andextend:withwhen:for conditional variable extension and file filtering.Documentation