feat(ext.generate): add optional features support for templates#768
Merged
Conversation
📝 WalkthroughWalkthroughAdds feature-driven conditional template generation: a new _process_features method validates feature configs, resolves requires to a fixpoint, collects feature variables/excludes/ignores, integrates into _generate, tightens template-module import handling, and adds demo templates plus extensive tests. ChangesFeature-driven template generation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
🧹 Nitpick comments (2)
demo/generate-features/templates/generate/webapp/Dockerfile (1)
4-4: Consider narrowing the build context.Line 4 copies the entire project tree into the image. For a template, that makes it easy for generated apps to accidentally bake in local junk or secrets unless a
.dockerignoreis added. Copying only the required files, or shipping a.dockerignore, would keep the demo safer and leaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/generate-features/templates/generate/webapp/Dockerfile` at line 4, The Dockerfile currently uses the broad "COPY . ." instruction which risks baking local junk/secrets into images; update the template by replacing the "COPY . ." step in the Dockerfile with an explicit list of COPY instructions that only add the application's build assets and manifest files (e.g., package.json, package-lock.json/yarn.lock, src/ or build/ folder as appropriate) and also include a template .dockerignore file in the template set that excludes node_modules, .git, .env, and other local artifacts—ensure the Dockerfile refers only to the files you explicitly copy and the .dockerignore is present in the generated project to prevent accidental inclusion.tests/data/templates/generate/test6/.generate.yml (1)
26-35: Makefeature2's disabled path explicit.
tests/data/templates/generate/test6/feature2-filereferencesfeature2_var, butfeature2_varis only added from the enabled block and this disabled block never ignores/excludes that file. In the defaulttest6path, the result depends on undefined-variable handling rather than the feature toggle itself. I'd either gatefeature2-filehere or assert the current disabled output explicitly in the tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/templates/generate/test6/.generate.yml` around lines 26 - 35, The disabled branch for the feature toggle "feature2" is missing an explicit exclusion for the template that references feature2_var, causing test output to depend on undefined-variable handling; update the "disabled.ignore" list to include the "feature2-file" pattern (e.g., add a glob matching feature2-file or '.*feature2-file.*') so that when feature2 is false the template is excluded, or alternatively add a disabled.variables entry providing a safe default for feature2_var; reference the "feature2", "feature2_var", and "feature2-file" symbols when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cement/ext/ext_generate.py`:
- Around line 37-57: The current loop in ext_generate.py treats missing
prerequisite states as disabled because it checks requires inline; change this
by resolving dependencies before assigning feature_states: build a dependency
graph from features (use the existing 'features', 'feature_names' and each
feature's 'requires'), perform a topological sort or implement a recursive
resolver function (e.g., resolve_feature_state(feature_name) that consults and
sets feature_states) to compute each feature's enabled/disabled value, and then
replace the inline requires-checking block (the unsatisfied logic that uses
'requires', 'feature_states', 'self.app.pargs.defaults' and logs via
'self.app.log.warning') with calls to the resolver so chains like feature_c ->
feature_b -> feature_a are correctly resolved regardless of YAML order.
- Around line 223-226: The except block currently swallows all ImportError
instances; change it to catch AttributeError and ModuleNotFoundError as e, and
if e is a ModuleNotFoundError ensure e.name matches the requested module
identifier (variable _from) before handling it—otherwise re-raise so dependency
import failures propagate; keep the existing debug message that uses mod_name
and '.'.join(mod_parts) and call app.log.debug(msg) only when the missing module
is the requested one.
In `@demo/generate-features/README.md`:
- Around line 70-96: In demo/generate-features/README.md update the three fenced
code blocks that show the project tree outputs so each opening fence is ```text
instead of ``` (the blocks containing the Docker-enabled tree, the "With docker
disabled" tree, and the "With docker enabled, docker_compose disabled" tree);
this will satisfy markdownlint MD040 by explicitly marking the code blocks as
plain text.
In `@demo/generate-features/templates/generate/webapp/Dockerfile`:
- Around line 3-6: Create and switch to an unprivileged user in the Dockerfile:
add steps to create a non-root user (e.g., "appuser"), chown -R /app to transfer
ownership after COPY, and add a USER appuser line before the existing CMD
["python","app.py"] so the container drops root privileges when starting the
application; keep WORKDIR /app and COPY . . as-is but ensure the chown happens
after the copy.
In `@tests/ext/test_ext_generate.py`:
- Around line 233-299: The feature dependency resolver in _process_features() is
order-dependent because it computes `unsatisfied` from the partially built
`feature_states`, causing dependents declared before their requirements to be
auto-disabled; make resolution order-independent by computing feature enablement
via a fixpoint/graph traversal: build a map of feature -> requires list (from
the features input), validate unknown requirements, then iterate (or DFS) to
compute each feature's final enabled state using default values and transitive
requires until no changes occur (or use topological order/detect cycles),
replacing the current `unsatisfied` logic that reads partial `feature_states`;
also add a new test case (similar to existing tests) where a feature appears
before its requirement in the YAML (out-of-order) and assert the dependent is
enabled when its requirement defaults to enabled to prevent regressions.
- Around line 225-230: In _process_features(), replace the two assert statements
that validate a feature's missing 'name' and unknown 'requires' with explicit
exceptions (e.g., raise ValueError with the same descriptive messages such as
"Required feature config key missing: name" and the existing unknown-requires
message), and update the test test_generate_features_missing_name (and related
tests that expect AssertionError) to expect ValueError (or your chosen exception
type) instead of AssertionError so validation works even under python -O.
---
Nitpick comments:
In `@demo/generate-features/templates/generate/webapp/Dockerfile`:
- Line 4: The Dockerfile currently uses the broad "COPY . ." instruction which
risks baking local junk/secrets into images; update the template by replacing
the "COPY . ." step in the Dockerfile with an explicit list of COPY instructions
that only add the application's build assets and manifest files (e.g.,
package.json, package-lock.json/yarn.lock, src/ or build/ folder as appropriate)
and also include a template .dockerignore file in the template set that excludes
node_modules, .git, .env, and other local artifacts—ensure the Dockerfile refers
only to the files you explicitly copy and the .dockerignore is present in the
generated project to prevent accidental inclusion.
In `@tests/data/templates/generate/test6/.generate.yml`:
- Around line 26-35: The disabled branch for the feature toggle "feature2" is
missing an explicit exclusion for the template that references feature2_var,
causing test output to depend on undefined-variable handling; update the
"disabled.ignore" list to include the "feature2-file" pattern (e.g., add a glob
matching feature2-file or '.*feature2-file.*') so that when feature2 is false
the template is excluded, or alternatively add a disabled.variables entry
providing a safe default for feature2_var; reference the "feature2",
"feature2_var", and "feature2-file" symbols when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 135d9fa6-13c6-4365-b0b9-22206454922a
📒 Files selected for processing (45)
CHANGELOG.mdCLAUDE.mdcement/ext/ext_generate.pydemo/generate-features/README.mddemo/generate-features/templates/generate/webapp/.generate.ymldemo/generate-features/templates/generate/webapp/Dockerfiledemo/generate-features/templates/generate/webapp/README.mddemo/generate-features/templates/generate/webapp/app.pydemo/generate-features/templates/generate/webapp/docker-compose.ymltests/data/templates/generate/test10/.generate.ymltests/data/templates/generate/test10/take-metests/data/templates/generate/test11/.generate.ymltests/data/templates/generate/test11/feature1-onlytests/data/templates/generate/test11/feature2-onlytests/data/templates/generate/test11/take-metests/data/templates/generate/test12/.generate.ymltests/data/templates/generate/test12/take-metests/data/templates/generate/test13/.generate.ymltests/data/templates/generate/test13/take-metests/data/templates/generate/test14/.generate.ymltests/data/templates/generate/test14/feature1-onlytests/data/templates/generate/test14/feature2-onlytests/data/templates/generate/test14/feature3-onlytests/data/templates/generate/test14/take-metests/data/templates/generate/test6/.generate.ymltests/data/templates/generate/test6/feature1-filetests/data/templates/generate/test6/feature1-onlytests/data/templates/generate/test6/feature2-filetests/data/templates/generate/test6/feature2-onlytests/data/templates/generate/test6/no-feature1tests/data/templates/generate/test6/take-metests/data/templates/generate/test7/.generate.ymltests/data/templates/generate/test7/feature1-filetests/data/templates/generate/test7/feature1-onlytests/data/templates/generate/test7/feature2-filetests/data/templates/generate/test7/feature2-onlytests/data/templates/generate/test7/no-feature1tests/data/templates/generate/test7/take-metests/data/templates/generate/test8/.generate.ymltests/data/templates/generate/test8/take-metests/data/templates/generate/test9/.generate.ymltests/data/templates/generate/test9/feature1-onlytests/data/templates/generate/test9/feature2-onlytests/data/templates/generate/test9/take-metests/ext/test_ext_generate.py
Comment on lines
+70
to
+96
| ``` | ||
| /tmp/myproject/ | ||
| ├── README.md | ||
| ├── app.py | ||
| ├── Dockerfile | ||
| └── docker-compose.yml | ||
| ``` | ||
|
|
||
| **With docker disabled** (docker_compose is auto-disabled via `requires`): | ||
|
|
||
| ``` | ||
| /tmp/myproject/ | ||
| ├── README.md | ||
| └── app.py | ||
| ``` | ||
|
|
||
| No Dockerfile, no docker-compose.yml — disabling docker automatically | ||
| disables docker_compose because it requires docker. | ||
|
|
||
| **With docker enabled, docker_compose disabled**: | ||
|
|
||
| ``` | ||
| /tmp/myproject/ | ||
| ├── README.md | ||
| ├── app.py | ||
| └── Dockerfile | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to the tree code fences.
The three fenced blocks starting at Line 70, Line 80, and Line 91 are missing a language tag, so markdownlint will keep flagging MD040 here. Using text is enough.
Suggested fix
-```
+```text
/tmp/myproject/
├── README.md
├── app.py
├── Dockerfile
└── docker-compose.yml@@
- +text
/tmp/myproject/
├── README.md
└── app.py
@@
-```
+```text
/tmp/myproject/
├── README.md
├── app.py
└── Dockerfile
</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
[warning] 80-80: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
[warning] 91-91: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @demo/generate-features/README.md around lines 70 - 96, In
demo/generate-features/README.md update the three fenced code blocks that show
the project tree outputs so each opening fence is text instead of (the
blocks containing the Docker-enabled tree, the "With docker disabled" tree, and
the "With docker enabled, docker_compose disabled" tree); this will satisfy
markdownlint MD040 by explicitly marking the code blocks as plain text.
</details>
<!-- fingerprinting:phantom:poseidon:grasshopper -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+3
to
+6
| WORKDIR /app | ||
| COPY . . | ||
|
|
||
| CMD ["python", "app.py"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the Dockerfile exists and read its full content
find demo -name "Dockerfile" -type f | head -20Repository: datafolklabs/cement
Length of output: 122
🏁 Script executed:
# Look for the specific file mentioned
cat demo/generate-features/templates/generate/webapp/DockerfileRepository: datafolklabs/cement
Length of output: 149
🏁 Script executed:
# Check for .dockerignore in the same directory
ls -la demo/generate-features/templates/generate/webapp/Repository: datafolklabs/cement
Length of output: 473
Drop root before starting the app.
The template never switches away from the default root user, so every generated container runs with full privileges. Add an unprivileged user and hand off /app before CMD.
Suggested fix
FROM python:{{ python_version }}-slim
WORKDIR /app
COPY . .
+RUN useradd --create-home --shell /usr/sbin/nologin appuser \
+ && chown -R appuser:appuser /app
+USER appuser
CMD ["python", "app.py"]📝 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
| WORKDIR /app | |
| COPY . . | |
| CMD ["python", "app.py"] | |
| FROM python:{{ python_version }}-slim | |
| WORKDIR /app | |
| COPY . . | |
| RUN useradd --create-home --shell /usr/sbin/nologin appuser \ | |
| && chown -R appuser:appuser /app | |
| USER appuser | |
| CMD ["python", "app.py"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@demo/generate-features/templates/generate/webapp/Dockerfile` around lines 3 -
6, Create and switch to an unprivileged user in the Dockerfile: add steps to
create a non-root user (e.g., "appuser"), chown -R /app to transfer ownership
after COPY, and add a USER appuser line before the existing CMD
["python","app.py"] so the container drops root privileges when starting the
application; keep WORKDIR /app and COPY . . as-is but ensure the chown happens
after the copy.
Comment on lines
+233
to
+299
| def test_generate_features_requires_satisfied(tmp): | ||
| # test9: feature1=true (default), feature2=true (default), feature2 requires feature1 | ||
| argv = ['generate', 'test9', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| app.run() | ||
|
|
||
| # both features enabled, both files present | ||
| assert exists_join(tmp.dir, 'take-me') | ||
| assert exists_join(tmp.dir, 'feature1-only') | ||
| assert exists_join(tmp.dir, 'feature2-only') | ||
|
|
||
|
|
||
| def test_generate_features_requires_not_satisfied(tmp): | ||
| # test11: feature1=false (default), feature2=true (default) but requires feature1 | ||
| # feature2 should be auto-disabled because feature1 is off | ||
| argv = ['generate', 'test11', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| app.run() | ||
|
|
||
| assert exists_join(tmp.dir, 'take-me') | ||
|
|
||
| # feature1 disabled: feature1-only ignored | ||
| assert not exists_join(tmp.dir, 'feature1-only') | ||
|
|
||
| # feature2 auto-disabled via requires: feature2-only ignored | ||
| assert not exists_join(tmp.dir, 'feature2-only') | ||
|
|
||
|
|
||
| def test_generate_features_requires_unknown(tmp): | ||
| # test10: feature requires a nonexistent feature | ||
| argv = ['generate', 'test10', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| with raises(AssertionError, match="requires unknown feature"): | ||
| app.run() | ||
|
|
||
|
|
||
| def test_generate_features_minimal(tmp): | ||
| # test13: feature with only name and default, no enabled/disabled blocks | ||
| argv = ['generate', 'test13', 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'), 'r') as f: | ||
| res = f.read() | ||
| assert 'myapp' in res | ||
|
|
||
|
|
||
| def test_generate_features_transitive_requires(tmp): | ||
| # test14: feature1=false, feature2=true requires feature1, | ||
| # feature3=true requires feature2 | ||
| # disabling feature1 should cascade: feature2 disabled, then feature3 disabled | ||
| argv = ['generate', 'test14', tmp.dir, '--defaults'] | ||
|
|
||
| with GenerateApp(argv=argv) as app: | ||
| app.run() | ||
|
|
||
| assert exists_join(tmp.dir, 'take-me') | ||
|
|
||
| # all three features should be disabled via transitive requires | ||
| assert not exists_join(tmp.dir, 'feature1-only') | ||
| assert not exists_join(tmp.dir, 'feature2-only') | ||
| assert not exists_join(tmp.dir, 'feature3-only') |
There was a problem hiding this comment.
Dependency resolution is still order-sensitive.
The new requires coverage only uses fixtures where prerequisites are declared earlier in YAML. _process_features() resolves unsatisfied from the partially built feature_states map, so a dependent that appears before its requirement will be auto-disabled incorrectly even if the requirement defaults to enabled. Please add an out-of-order regression case and make the resolver order-independent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ext/test_ext_generate.py` around lines 233 - 299, The feature
dependency resolver in _process_features() is order-dependent because it
computes `unsatisfied` from the partially built `feature_states`, causing
dependents declared before their requirements to be auto-disabled; make
resolution order-independent by computing feature enablement via a
fixpoint/graph traversal: build a map of feature -> requires list (from the
features input), validate unknown requirements, then iterate (or DFS) to compute
each feature's final enabled state using default values and transitive requires
until no changes occur (or use topological order/detect cycles), replacing the
current `unsatisfied` logic that reads partial `feature_states`; also add a new
test case (similar to existing tests) where a feature appears before its
requirement in the YAML (out-of-order) and assert the dependent is enabled when
its requirement defaults to enabled to prevent regressions.
Allow .generate.yml templates to declare optional features with
conditional variables, exclude patterns, and ignore patterns based
on enabled/disabled state. Features support dependency resolution
via `requires` with automatic transitive cascading — disabled
dependencies skip prompting for dependent features entirely.
Resolution is order-independent: a feature may declare `requires`
against another feature defined later in the YAML.
Validation failures (missing `name`, unknown `requires` target)
raise ValueError so they survive `python -O` (asserts get stripped).
Also tighten setup_template_items: separate ModuleNotFoundError
from AttributeError and only swallow when the missing module is the
template module being imported — transitive ModuleNotFoundError
raised from inside the user's template module now propagates
instead of being silently logged. Correct the variables default
from {} to [].
Related:
- Resolves Issue #743
derks
force-pushed
the
feat/generate-optional-features
branch
from
e11c7d8 to
853e878
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cement/ext/ext_generate.py (1)
29-34: ⚡ Quick winUse the repo’s full-form typing style in the new helper signature.
The new annotations use built-in generics (
list[...],dict[...]), but this repo’s Python style requiresList[...]/Dict[...]forms. Please align the new helper signature while touching this code so it stays consistent with the strict typing rules. As per coding guidelines "Type annotations must use full form:Dict[str, Any],Optional[str],List[str],Tuple[int, str]".🤖 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 29 - 34, Update the _process_features helper signature to use the repo’s full-form typing (replace built-in generics with typing ones): change parameters typed as list[dict[str, Any]] and dict[str, Any] to List[Dict[str, Any]] and Dict[str, Any]; ensure you import List, Dict, Any from typing at the top of the module if not already present and adjust any sibling annotations in the same file to match this style (refer to the _process_features definition to locate the change).
🤖 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 50-90: The interactive flow currently prompts every feature
up-front (FeaturePrompt in the pass 1 loop) which leads to prompting dependents
even when their prerequisites later get declined; change to lazy prompting so
prerequisites are resolved before prompting dependents: stop prompting in the
initial pass (when self.app.pargs.defaults is False) — only initialise
feature_states with default booleans or an explicit "undecided" marker — then in
the pass 2 cascade loop (the while changed loop that inspects
feature.get('requires', [])) when you encounter a required feature whose state
is undecided and interactive, instantiate and run the FeaturePrompt for that
specific required feature (using the same prompt text/default logic currently in
the FeaturePrompt inner class), set its feature_states entry based on the
answer, and continue dependency resolution; finally ensure dependents are never
prompted if a prerequisite resolves to False, and add one interactive regression
test exercising a declined prerequisite that would previously still prompt its
dependent to assert no prompt is shown for the dependent.
- Around line 124-126: The code uses g_config.get('variables', []),
g_config.get('exclude', []), and g_config.get('ignore', []) which still yield
None if YAML explicitly sets those keys to null; change these to use the
coalescing pattern (e.g., g_config.get('variables') or []) for vars,
exclude_list, and ignore_list so the subsequent loop (for defined_var in vars)
and any consumers are safe when the YAML contains null values, matching the
existing handling used for feature blocks.
- Around line 29-34: Rename the parameter named vars in the _process_features
method to variables and also rename the local variable currently assigned to
vars (the one flagged by Ruff) to variables so you no longer shadow Python's
built-in vars(); update all references inside _process_features to use variables
instead of vars (including any iteration, lookups, or passes to other functions)
to fix the A002/A001 lint violations.
---
Nitpick comments:
In `@cement/ext/ext_generate.py`:
- Around line 29-34: Update the _process_features helper signature to use the
repo’s full-form typing (replace built-in generics with typing ones): change
parameters typed as list[dict[str, Any]] and dict[str, Any] to List[Dict[str,
Any]] and Dict[str, Any]; ensure you import List, Dict, Any from typing at the
top of the module if not already present and adjust any sibling annotations in
the same file to match this style (refer to the _process_features definition to
locate the change).
🪄 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: f823932c-88b2-4e0c-951e-4037917d2421
📒 Files selected for processing (51)
CHANGELOG.mdCLAUDE.mdcement/ext/ext_generate.pydemo/generate-features/README.mddemo/generate-features/templates/generate/webapp/.dockerignoredemo/generate-features/templates/generate/webapp/.generate.ymldemo/generate-features/templates/generate/webapp/Dockerfiledemo/generate-features/templates/generate/webapp/README.mddemo/generate-features/templates/generate/webapp/app.pydemo/generate-features/templates/generate/webapp/docker-compose.ymltests/data/bad_template_module/__init__.pytests/data/templates/generate/test10/.generate.ymltests/data/templates/generate/test10/take-metests/data/templates/generate/test11/.generate.ymltests/data/templates/generate/test11/feature1-onlytests/data/templates/generate/test11/feature2-onlytests/data/templates/generate/test11/take-metests/data/templates/generate/test12/.generate.ymltests/data/templates/generate/test12/take-metests/data/templates/generate/test13/.generate.ymltests/data/templates/generate/test13/take-metests/data/templates/generate/test14/.generate.ymltests/data/templates/generate/test14/feature1-onlytests/data/templates/generate/test14/feature2-onlytests/data/templates/generate/test14/feature3-onlytests/data/templates/generate/test14/take-metests/data/templates/generate/test15/.generate.ymltests/data/templates/generate/test15/feature_a-onlytests/data/templates/generate/test15/feature_b-onlytests/data/templates/generate/test15/take-metests/data/templates/generate/test6/.generate.ymltests/data/templates/generate/test6/feature1-filetests/data/templates/generate/test6/feature1-onlytests/data/templates/generate/test6/feature2-filetests/data/templates/generate/test6/feature2-onlytests/data/templates/generate/test6/no-feature1tests/data/templates/generate/test6/take-metests/data/templates/generate/test7/.generate.ymltests/data/templates/generate/test7/feature1-filetests/data/templates/generate/test7/feature1-onlytests/data/templates/generate/test7/feature2-filetests/data/templates/generate/test7/feature2-onlytests/data/templates/generate/test7/no-feature1tests/data/templates/generate/test7/take-metests/data/templates/generate/test8/.generate.ymltests/data/templates/generate/test8/take-metests/data/templates/generate/test9/.generate.ymltests/data/templates/generate/test9/feature1-onlytests/data/templates/generate/test9/feature2-onlytests/data/templates/generate/test9/take-metests/ext/test_ext_generate.py
✅ Files skipped from review due to trivial changes (39)
- tests/data/templates/generate/test9/feature2-only
- tests/data/templates/generate/test6/feature2-only
- tests/data/templates/generate/test8/take-me
- tests/data/templates/generate/test7/take-me
- tests/data/templates/generate/test12/take-me
- tests/data/templates/generate/test13/.generate.yml
- tests/data/templates/generate/test6/no-feature1
- demo/generate-features/templates/generate/webapp/.dockerignore
- demo/generate-features/templates/generate/webapp/docker-compose.yml
- tests/data/templates/generate/test15/feature_a-only
- tests/data/templates/generate/test15/feature_b-only
- tests/data/templates/generate/test6/feature1-only
- tests/data/templates/generate/test10/take-me
- tests/data/templates/generate/test6/feature2-file
- tests/data/templates/generate/test14/feature1-only
- tests/data/templates/generate/test9/take-me
- tests/data/templates/generate/test11/feature2-only
- tests/data/templates/generate/test7/feature1-only
- tests/data/templates/generate/test7/feature2-file
- tests/data/templates/generate/test7/feature1-file
- tests/data/templates/generate/test14/feature2-only
- tests/data/templates/generate/test6/take-me
- tests/data/templates/generate/test13/take-me
- demo/generate-features/templates/generate/webapp/.generate.yml
- tests/data/templates/generate/test11/feature1-only
- tests/data/templates/generate/test7/feature2-only
- tests/data/templates/generate/test11/take-me
- tests/data/templates/generate/test7/no-feature1
- tests/data/templates/generate/test6/feature1-file
- tests/data/templates/generate/test14/take-me
- tests/data/templates/generate/test6/.generate.yml
- tests/data/templates/generate/test14/feature3-only
- tests/data/templates/generate/test9/.generate.yml
- CLAUDE.md
- tests/data/templates/generate/test7/.generate.yml
- tests/data/templates/generate/test8/.generate.yml
- demo/generate-features/README.md
- tests/data/templates/generate/test9/feature1-only
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/data/templates/generate/test12/.generate.yml
- tests/data/templates/generate/test11/.generate.yml
- tests/data/templates/generate/test10/.generate.yml
- demo/generate-features/templates/generate/webapp/app.py
- tests/data/templates/generate/test14/.generate.yml
Comment on lines
+29
to
+34
| def _process_features(self, | ||
| features: list[dict[str, Any]], | ||
| vars: list[dict[str, Any]], | ||
| exclude_list: list[str], | ||
| ignore_list: list[str], | ||
| data: dict[str, Any]) -> None: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "ext_generate.py"Repository: datafolklabs/cement
Length of output: 1941
🏁 Script executed:
cat -n cement/ext/ext_generate.py | head -150Repository: datafolklabs/cement
Length of output: 1941
🏁 Script executed:
rg -n "def _process_features|def _generate" cement/ext/ext_generate.py -A 10Repository: datafolklabs/cement
Length of output: 1186
🏁 Script executed:
cat -n cement/ext/ext_generate.py | sed -n '120,130p'Repository: datafolklabs/cement
Length of output: 601
🏁 Script executed:
rg -n "vars\b" cement/ext/ext_generate.py | head -20Repository: datafolklabs/cement
Length of output: 446
Rename vars parameter and variable before merge.
The parameter vars on line 31 and the variable assignment on line 124 shadow Python's built-in vars() function. Ruff flags these as A002 and A001 violations respectively. Renaming both to variables (or similar) fixes the lint failures and improves readability since the data actually represents variables. This is required per the coding guideline: "All code must pass ruff linting (v0.3.2+)".
🧰 Tools
🪛 Ruff (0.15.12)
[error] 31-31: Function argument vars is shadowing a Python builtin
(A002)
🤖 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 29 - 34, Rename the parameter named
vars in the _process_features method to variables and also rename the local
variable currently assigned to vars (the one flagged by Ruff) to variables so
you no longer shadow Python's built-in vars(); update all references inside
_process_features to use variables instead of vars (including any iteration,
lookups, or passes to other functions) to fix the A002/A001 lint violations.
Comment on lines
+50
to
+90
| # pass 1: compute tentative enabled-state per feature from default / | ||
| # prompt, ignoring requires. Order-independent. | ||
| feature_states: dict[str, bool] = {} | ||
| for feature in features: | ||
| name = feature['name'] | ||
| default = bool(feature.get('default', False)) | ||
| if self.app.pargs.defaults: | ||
| feature_states[name] = default | ||
| else: | ||
| default_hint = 'Y/n' if default else 'y/N' # pragma: nocover | ||
| default_val = 'y' if default else 'n' # pragma: nocover | ||
|
|
||
| class FeaturePrompt(shell.Prompt): # pragma: nocover | ||
| class Meta: | ||
| text = f"Enable Feature: {name} [{default_hint}]:" | ||
| default = default_val | ||
|
|
||
| p = FeaturePrompt(auto=False) # pragma: nocover | ||
| val: str = p.prompt() or default_val # pragma: nocover | ||
| feature_states[name] = val.lower() == 'y' # pragma: nocover | ||
|
|
||
| # pass 2: cascade-disable features whose requires are unsatisfied; | ||
| # iterate to a fixpoint so transitive chains resolve regardless of | ||
| # YAML ordering (a feature can declare a `requires` against another | ||
| # feature defined later in the list). | ||
| changed = True | ||
| while changed: | ||
| changed = False | ||
| for feature in features: | ||
| name = feature['name'] | ||
| if not feature_states[name]: | ||
| continue | ||
| for req in feature.get('requires', []): | ||
| if not feature_states[req]: | ||
| self.app.log.warning( | ||
| f"Feature '{name}' disabled (requires: {req})" | ||
| ) | ||
| feature_states[name] = False | ||
| changed = True | ||
| break | ||
|
|
There was a problem hiding this comment.
Skip dependent feature prompts until prerequisites are resolved.
The new two-pass flow still prompts every feature in interactive mode before requires is applied, so declining a prerequisite can still ask the user about dependents and then discard that answer in pass 2. That misses the PR goal of skipping prompts for disabled dependency chains. Please resolve prerequisites first, or prompt lazily during dependency resolution, and add one interactive regression test for that 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 50 - 90, The interactive flow
currently prompts every feature up-front (FeaturePrompt in the pass 1 loop)
which leads to prompting dependents even when their prerequisites later get
declined; change to lazy prompting so prerequisites are resolved before
prompting dependents: stop prompting in the initial pass (when
self.app.pargs.defaults is False) — only initialise feature_states with default
booleans or an explicit "undecided" marker — then in the pass 2 cascade loop
(the while changed loop that inspects feature.get('requires', [])) when you
encounter a required feature whose state is undecided and interactive,
instantiate and run the FeaturePrompt for that specific required feature (using
the same prompt text/default logic currently in the FeaturePrompt inner class),
set its feature_states entry based on the answer, and continue dependency
resolution; finally ensure dependents are never prompted if a prerequisite
resolves to False, and add one interactive regression test exercising a declined
prerequisite that would previously still prompt its dependent to assert no
prompt is shown for the dependent.
Replace the two-pass (compute-then-cascade) feature resolver in _process_features with a recursive _resolve(name) helper. Behaviorally this changes only the interactive flow: a feature's prompt is now reached lazily — only after every one of its `requires` has resolved truthy. Previously every feature was prompted up-front in pass 1, so declining feature_a still produced a prompt for feature_b (which requires feature_a) before pass 2 cascaded the disable. Addresses review feedback. The --defaults code path and all existing test cases (in-order / out-of-order / transitive / requires-not-satisfied / unknown-requires / missing-name) behave identically. No new test surface — the prompt branch is unchanged structurally and remains pragma: nocover (cement has no harness for driving interactive prompts).
`g_config.get('variables', [])` only catches the *missing* key — if
the YAML explicitly sets `variables: null` (or `exclude: null` /
`ignore: null`), `.get` returns the literal None and the next line
(ignore_list.append, for defined_var in vars) crashes. Switch to the
`.get(key) or []` coalescing pattern so an explicit null is treated
the same as omission. Addresses review feedback.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cement/ext/ext_generate.py (1)
31-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename
varsto avoid shadowing the Python builtin (Ruff A002/A001).Both the
_process_featuresparameter at Line 31 and the local at Line 125 still shadow the builtinvars()and continue to fail Ruff. Renaming both tovariables(and updating the references at lines 100, 147 accordingly) clears the lint violations and is required by the project's PEP8/ruff gating.♻️ Proposed rename
def _process_features(self, features: list[dict[str, Any]], - vars: list[dict[str, Any]], + variables: list[dict[str, Any]], exclude_list: list[str], ignore_list: list[str], data: dict[str, Any]) -> None: @@ - vars.extend(block.get('variables', [])) + variables.extend(block.get('variables', [])) exclude_list.extend(block.get('exclude', [])) ignore_list.extend(block.get('ignore', [])) @@ - vars = g_config.get('variables') or [] + variables = g_config.get('variables') or [] exclude_list = g_config.get('exclude') or [] ignore_list = g_config.get('ignore') or [] @@ - self._process_features(features, vars, exclude_list, - ignore_list, data) + self._process_features(features, variables, exclude_list, + ignore_list, data) @@ - for defined_var in vars: + for defined_var in variables:As per coding guidelines: "Run ruff linting on Python files to ensure PEP8 compliance".
Also applies to: 125-125
🤖 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` at line 31, The parameter named vars in the _process_features signature and the local variable named vars (used inside _process_features) shadow the Python builtin vars() and trigger Ruff A002/A001; rename the parameter to variables and the local to variables (or variables_local) and update all references inside the function (calls/usages currently around where vars is referenced, e.g., the usages previously at lines ~100 and ~147) to the new name so the function _process_features and its internal logic compile and pass linting.
🧹 Nitpick comments (1)
cement/ext/ext_generate.py (1)
256-256: 💤 Low valueNit: prefer unpacking over list concatenation (RUF005).
- expected = '.'.join(mod_parts + [mod_name]) + expected = '.'.join([*mod_parts, mod_name])As per coding guidelines: "Run ruff linting on Python files to ensure PEP8 compliance".
🤖 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` at line 256, Replace the list concatenation used to build the module path in the assignment expected = '.'.join(mod_parts + [mod_name]) with sequence unpacking to satisfy the linter; e.g. construct the joinable list as '.'.join([*mod_parts, mod_name]) (referencing the variables mod_parts and mod_name where expected is computed).
🤖 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.
Duplicate comments:
In `@cement/ext/ext_generate.py`:
- Line 31: The parameter named vars in the _process_features signature and the
local variable named vars (used inside _process_features) shadow the Python
builtin vars() and trigger Ruff A002/A001; rename the parameter to variables and
the local to variables (or variables_local) and update all references inside the
function (calls/usages currently around where vars is referenced, e.g., the
usages previously at lines ~100 and ~147) to the new name so the function
_process_features and its internal logic compile and pass linting.
---
Nitpick comments:
In `@cement/ext/ext_generate.py`:
- Line 256: Replace the list concatenation used to build the module path in the
assignment expected = '.'.join(mod_parts + [mod_name]) with sequence unpacking
to satisfy the linter; e.g. construct the joinable list as '.'.join([*mod_parts,
mod_name]) (referencing the variables mod_parts and mod_name where expected is
computed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f621ebb9-d932-4848-a5f0-72a0011cf0e7
📒 Files selected for processing (1)
cement/ext/ext_generate.py
4 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
1 participant
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.
Allow .generate.yml templates to declare optional features with conditional variables, exclude patterns, and ignore patterns based on enabled/disabled state. Features support dependency resolution via
requireswith automatic transitive cascading — disabled dependencies skip prompting for dependent features entirely.Also fix setup_template_items to catch ImportError and correct the variables default from {} to [].
Related:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Assets
Tests