feat: support template_dirs config settings#760
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughApp.Meta.template_dirs can now be overridden from app config; config-provided comma-separated strings are normalized to trimmed lists and synced to Meta. Five tests verify list override, string parsing (including dropping empty tokens), token substitution, and precedence. CHANGELOG, CONTRIBUTORS, and planning metadata were updated. ChangesTemplate dirs override
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Config
participant TemplateHandler
User->>App: Start application
App->>Config: Check for 'template_dirs' in config
alt 'template_dirs' found in config
Config-->>App: Return config 'template_dirs' (str or list)
App->>App: Normalize (if str -> split/trim) and write back to config
App->>App: Set self._meta.template_dirs from normalized list
else Not found
App->>App: Use Meta.template_dirs
end
App->>TemplateHandler: Setup with final template_dirs list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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: 1
🧹 Nitpick comments (1)
cement/core/foundation.py (1)
525-535: Fix typo in comment.There's a typo in the comment: "becuase" should be "because".
- # reverse it becuase it will be reverse again (keep user preference) + # reverse it because it will be reversed again (keep user preference)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)CONTRIBUTORS.md(1 hunks)cement/core/foundation.py(3 hunks)tests/core/test_foundation.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
cement/core/foundation.py
[error] 1554-1554: Instance of 'Meta' has no 'core_user_template_dirs' member
(E1101)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (8)
cement/core/foundation.py (4)
500-500: LGTM: Adding template_dirs to core_meta_override list.This correctly enables configuration override for the template_dirs meta attribute, consistent with other overridable meta options.
525-535: Excellent implementation of config override logic.The logic correctly:
- Checks for template_dirs in the config section
- Handles both list and comma-separated string formats
- Reverses the list to maintain user preference order
- Follows established patterns from similar config overrides
544-553: LGTM: Config-aware template directory selection.The conditional logic properly prioritizes config-provided template_dirs over meta-defined ones while maintaining the established directory precedence order.
1554-1554: The static analysis hint is a false positive.The static analysis tool flagged
core_user_template_dirsas missing, but this attribute is clearly defined in theMetaclass at lines 723-726. This is a false positive and can be safely ignored.🧰 Tools
🪛 Pylint (3.3.7)
[error] 1554-1554: Instance of 'Meta' has no 'core_user_template_dirs' member
(E1101)
tests/core/test_foundation.py (3)
782-795: Excellent test coverage for config override functionality.This test properly verifies that:
- Config template_dirs override meta template_dirs
- Original meta paths are excluded when config overrides exist
- Both list formats work correctly
The test design is clean and assertions are appropriate.
797-809: Good test for string format conversion.This test correctly validates the comma-separated string parsing functionality, including handling of whitespace around commas. The test design matches the expected behavior from the implementation.
811-823: Thorough test for template_dir and template_dirs interaction.This test ensures that both singular
template_dirand pluraltemplate_dirssettings work together correctly, which is important for backward compatibility and flexibility.CONTRIBUTORS.md (1)
26-26: LGTM: Welcome new contributor!Appropriate addition of the new contributor following the established format.
|
@TomFreudenberg this PR is ready for review if you are interested to test it. I believe it solves your requests in Issue #746 - I'm not going to merge it just yet, I need to review some things. Particularly, on whether the config This also adds you to the CONTRIBUTORS.md file that you mentioned in a previous PR. Thank you for your patience. |
|
Dear @derks sorry for being very late on your feedback and your effort. I had some private issues and it took a while for me to continue work. I will come up to you with feedback soon. Kindly regards |
|
@TomFreudenberg good to hear from you, and sorry to hear the unfortunate news. I have fallen behind in active dev, so this is a good reminder for me as well. No rush, its always good to have your feedback on Cement. |
Quick task scaffolding for the PR #760 rewrite — PLAN.md captures the scope decisions (use core_meta_override, leave _setup_template_handler untouched, replace-not-extend semantics), and SUMMARY.md records the final state including verification results and the pre-push local-only status pending user review.
derks
force-pushed
the
feat/support-template-dirs-meta-override
branch
from
0cf3873 to
4d61a86
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/core/foundation.py`:
- Around line 1466-1475: The normalization block for template_dirs uses
self._meta.config_section in the guard but calls self.config.get and
self.config.set with label, causing normalization to miss the actual config
section when label != self._meta.config_section; update the calls to use
self._meta.config_section (or consistently use label throughout) so the get and
set operate on the same section checked by the guard — specifically adjust the
usages of self.config.get(label, 'template_dirs') and self.config.set(label,
'template_dirs', dir_list') to reference self._meta.config_section (and ensure
_meta.template_dirs is updated accordingly).
- Around line 1470-1472: When `dirs` is a comma-separated string, the current
split keeps empty tokens (e.g. trailing commas); in the block handling `if
type(dirs) is str:` (variable names `dirs` and `dir_list` in
cement.core.foundation.py) change the construction of `dir_list` to strip each
token and drop empty/falsy results (i.e., build `dir_list` from tokens where
`token.strip()` is non-empty) so accidental empty path entries are not included
in template path resolution.
In `@tests/core/test_foundation.py`:
- Around line 844-847: The Meta class currently uses mutable list literals for
core_system_template_dirs, core_user_template_dirs, and template_dirs which Ruff
flags; replace those list literals with immutable tuples (e.g., ('/sys/dir',))
or set them to None and initialize immutable sequences at runtime so they are
not mutable class defaults; update the attributes core_system_template_dirs,
core_user_template_dirs, and template_dirs accordingly (leave template_dir
as-is) to satisfy RUF012 and re-run the ruff/comply checks.
🪄 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: 7725a4f3-2a10-49a3-a9f9-ac715597b2c4
📒 Files selected for processing (7)
.planning/STATE.md.planning/quick/260507-to4-rewrite-pr-760-template-dirs-config-over/260507-to4-PLAN.md.planning/quick/260507-to4-rewrite-pr-760-template-dirs-config-over/260507-to4-SUMMARY.mdCHANGELOG.mdCONTRIBUTORS.mdcement/core/foundation.pytests/core/test_foundation.py
✅ Files skipped from review due to trivial changes (3)
- CONTRIBUTORS.md
- .planning/quick/260507-to4-rewrite-pr-760-template-dirs-config-over/260507-to4-PLAN.md
- .planning/STATE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
| core_system_template_dirs = ['/sys/dir'] | ||
| core_user_template_dirs = ['/usr/dir'] | ||
| template_dir = '/single/dir' | ||
| template_dirs = ['/meta/dir/should/be/replaced'] |
There was a problem hiding this comment.
Avoid mutable list defaults in nested Meta class attributes (RUF012).
core_system_template_dirs, core_user_template_dirs, and template_dirs are list literals on class attrs. Ruff flags this pattern and it can break compliance gates.
Suggested fix
- core_system_template_dirs = ['/sys/dir']
- core_user_template_dirs = ['/usr/dir']
+ core_system_template_dirs = ('/sys/dir',)
+ core_user_template_dirs = ('/usr/dir',)
template_dir = '/single/dir'
- template_dirs = ['/meta/dir/should/be/replaced']
+ template_dirs = ('/meta/dir/should/be/replaced',)As per coding guidelines, "**/*.py: Run ruff linting via make comply-ruff and auto-fix issues with make comply-ruff-fix" and "Maintain 100% PEP8 compliance via ruff ... enforced as absolute quality gates."
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 844-844: Mutable default value for class attribute
(RUF012)
[warning] 845-845: Mutable default value for class attribute
(RUF012)
[warning] 847-847: Mutable default value for class attribute
(RUF012)
🤖 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/core/test_foundation.py` around lines 844 - 847, The Meta class
currently uses mutable list literals for core_system_template_dirs,
core_user_template_dirs, and template_dirs which Ruff flags; replace those list
literals with immutable tuples (e.g., ('/sys/dir',)) or set them to None and
initialize immutable sequences at runtime so they are not mutable class
defaults; update the attributes core_system_template_dirs,
core_user_template_dirs, and template_dirs accordingly (leave template_dir
as-is) to satisfy RUF012 and re-run the ruff/comply checks.
Quick task scaffolding for the PR #760 rewrite — PLAN.md captures the scope decisions (use core_meta_override, leave _setup_template_handler untouched, replace-not-extend semantics), and SUMMARY.md records the final state including verification results and the pre-push local-only status pending user review.
derks
force-pushed
the
feat/support-template-dirs-meta-override
branch
from
4d61a86 to
0df5e6a
Compare
Quick task scaffolding for the PR #760 rewrite — PLAN.md captures the scope decisions (use core_meta_override, leave _setup_template_handler untouched, replace-not-extend semantics), and SUMMARY.md records the final state including verification results and the pre-push local-only status pending user review.
derks
force-pushed
the
feat/support-template-dirs-meta-override
branch
from
0df5e6a to
abc076c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
cement/core/foundation.py (2)
1474-1474:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmpty path tokens produced by trailing/double commas still not filtered.
'a,,b,'.split(',')produces['a', '', 'b', '']; after.strip()the empty strings remain and become invalid entries in template path resolution.🐛 Proposed fix
- dir_list = [x.strip() for x in dirs.split(',')] + dir_list = [x.strip() for x in dirs.split(',') if x.strip()]🤖 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/core/foundation.py` at line 1474, The current creation of dir_list from dirs leaves empty tokens when there are trailing or consecutive commas; update the comprehension that builds dir_list (the one using dirs.split(',')) to discard empty/whitespace-only entries after stripping — i.e., filter tokens by checking x.strip() so only non-empty paths remain; adjust any downstream assumptions if necessary that rely on dir_list containing only valid, non-empty path strings.
1469-1481:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
labelvsself._meta.config_sectionmismatch still unresolved.Line 1469 guards on
self._meta.config_section, but lines 1470 and 1477 passlabel(self._meta.label) toconfig.get/config.set. When a caller explicitly sets aconfig_sectionthat differs from the app label, the guard checks the correct section but the read/write targets the wrong one — leaving_meta.template_dirsholding the raw string and the active section uncorrected.🐛 Proposed fix
- if 'template_dirs' in self.config.keys(self._meta.config_section): - dirs = self.config.get(label, 'template_dirs') + section = self._meta.config_section + if 'template_dirs' in self.config.keys(section): + dirs = self.config.get(section, 'template_dirs') # convert a comma-separated string to a list if type(dirs) is str: dir_list = [x.strip() for x in dirs.split(',')] # set the new template_dirs value in the config - self.config.set(label, 'template_dirs', dir_list) + self.config.set(section, 'template_dirs', dir_list) # also update _meta.template_dirs which the override loop # above set to the raw string self._meta.template_dirs = dir_list🤖 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/core/foundation.py` around lines 1469 - 1481, The guard checks self._meta.config_section but the code incorrectly uses label when calling self.config.get and self.config.set and when updating self._meta.template_dirs; change those calls to use the same section (self._meta.config_section) so read/write targets the guarded section and then update self._meta.template_dirs from the normalized list returned by self.config.get(self._meta.config_section, 'template_dirs') after converting comma strings to list.tests/core/test_foundation.py (1)
844-847:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRUF012: mutable list defaults in nested
Metaclass still unresolved.
core_system_template_dirs,core_user_template_dirs, andtemplate_dirsare mutable list literals as class attributes. Ruff flags all three, and the linter is an absolute quality gate per the coding guidelines.🔧 Proposed fix
- core_system_template_dirs = ['/sys/dir'] - core_user_template_dirs = ['/usr/dir'] template_dir = '/single/dir' - template_dirs = ['/meta/dir/should/be/replaced'] + core_system_template_dirs = ('/sys/dir',) + core_user_template_dirs = ('/usr/dir',) + template_dirs = ('/meta/dir/should/be/replaced',)As per coding guidelines,
**/*.py: Run ruff linting via make comply-ruff ... Maintain 100% PEP8 compliance via ruff ... enforced as absolute quality gates.🤖 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/core/test_foundation.py` around lines 844 - 847, The nested Meta class uses mutable list literals for core_system_template_dirs, core_user_template_dirs, and template_dirs which triggers RUF012; replace those list literals with immutable tuples (e.g., ('/sys/dir',), ('/usr/dir',), ('/meta/dir/should/be/replaced',)) or set them to None and initialize immutably elsewhere, and update any test assertions that expect list types if needed; the attributes to change are core_system_template_dirs, core_user_template_dirs, and template_dirs in the Meta class (leave template_dir as-is).
🤖 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/core/foundation.py`:
- Line 1474: The current creation of dir_list from dirs leaves empty tokens when
there are trailing or consecutive commas; update the comprehension that builds
dir_list (the one using dirs.split(',')) to discard empty/whitespace-only
entries after stripping — i.e., filter tokens by checking x.strip() so only
non-empty paths remain; adjust any downstream assumptions if necessary that rely
on dir_list containing only valid, non-empty path strings.
- Around line 1469-1481: The guard checks self._meta.config_section but the code
incorrectly uses label when calling self.config.get and self.config.set and when
updating self._meta.template_dirs; change those calls to use the same section
(self._meta.config_section) so read/write targets the guarded section and then
update self._meta.template_dirs from the normalized list returned by
self.config.get(self._meta.config_section, 'template_dirs') after converting
comma strings to list.
In `@tests/core/test_foundation.py`:
- Around line 844-847: The nested Meta class uses mutable list literals for
core_system_template_dirs, core_user_template_dirs, and template_dirs which
triggers RUF012; replace those list literals with immutable tuples (e.g.,
('/sys/dir',), ('/usr/dir',), ('/meta/dir/should/be/replaced',)) or set them to
None and initialize immutably elsewhere, and update any test assertions that
expect list types if needed; the attributes to change are
core_system_template_dirs, core_user_template_dirs, and template_dirs in the
Meta class (leave template_dir as-is).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfa1d94b-031e-4789-b51b-75f5338a9b3d
📒 Files selected for processing (7)
.planning/STATE.md.planning/quick/260507-to4-rewrite-pr-760-template-dirs-config-over/260507-to4-PLAN.md.planning/quick/260507-to4-rewrite-pr-760-template-dirs-config-over/260507-to4-SUMMARY.mdCHANGELOG.mdCONTRIBUTORS.mdcement/core/foundation.pytests/core/test_foundation.py
✅ Files skipped from review due to trivial changes (4)
- CONTRIBUTORS.md
- CHANGELOG.md
- .planning/quick/260507-to4-rewrite-pr-760-template-dirs-config-over/260507-to4-PLAN.md
- .planning/quick/260507-to4-rewrite-pr-760-template-dirs-config-over/260507-to4-SUMMARY.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .planning/STATE.md
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/core/test_foundation.py (1)
859-864:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMutable list defaults in
Metastill violate RUF012 (quality gate).Lines 861, 862, and 864 use
listliterals as class-attribute defaults in the nestedMetaclass. Ruff's RUF012 rule flags all three, which blocks themake comply-ruffquality gate.Replace with immutable tuples (the iteration in
_setup_template_handleraccepts any sequence):🔧 Proposed fix
class ThisTestApp(TestApp): class Meta: - core_system_template_dirs = ['/sys/dir'] - core_user_template_dirs = ['/usr/dir'] + core_system_template_dirs = ('/sys/dir',) + core_user_template_dirs = ('/usr/dir',) template_dir = '/single/dir' - template_dirs = ['/meta/dir/should/be/replaced'] + template_dirs = ('/meta/dir/should/be/replaced',)As per coding guidelines, "**/*.py: Run ruff linting via
make comply-ruff" and "Maintain 100% PEP8 compliance via ruff … enforced as absolute quality gates."🤖 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/core/test_foundation.py` around lines 859 - 864, ThisTestApp.Meta currently uses mutable list literals for core_system_template_dirs, core_user_template_dirs, and template_dirs which triggers Ruff RUF012; change those class-attribute defaults to immutable tuples (e.g. ('/sys/dir',) etc.) so the nested Meta uses tuples instead of lists—_setup_template_handler accepts any sequence so behavior is preserved; update the attributes core_system_template_dirs, core_user_template_dirs, and template_dirs in the Meta class accordingly.
🤖 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 `@tests/core/test_foundation.py`:
- Around line 859-864: ThisTestApp.Meta currently uses mutable list literals for
core_system_template_dirs, core_user_template_dirs, and template_dirs which
triggers Ruff RUF012; change those class-attribute defaults to immutable tuples
(e.g. ('/sys/dir',) etc.) so the nested Meta uses tuples instead of
lists—_setup_template_handler accepts any sequence so behavior is preserved;
update the attributes core_system_template_dirs, core_user_template_dirs, and
template_dirs in the Meta class accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f21830cf-baaf-4fb9-9597-99512053e187
📒 Files selected for processing (7)
.planning/STATE.md.planning/quick/260507-to4-rewrite-pr-760-template-dirs-config-over/260507-to4-PLAN.md.planning/quick/260507-to4-rewrite-pr-760-template-dirs-config-over/260507-to4-SUMMARY.mdCHANGELOG.mdCONTRIBUTORS.mdcement/core/foundation.pytests/core/test_foundation.py
✅ Files skipped from review due to trivial changes (3)
- CONTRIBUTORS.md
- CHANGELOG.md
- .planning/quick/260507-to4-rewrite-pr-760-template-dirs-config-over/260507-to4-PLAN.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .planning/STATE.md
- cement/core/foundation.py
- .planning/quick/260507-to4-rewrite-pr-760-template-dirs-config-over/260507-to4-SUMMARY.md
Add 'template_dirs' to Meta.core_meta_override so config sections can
replace App.Meta.template_dirs at setup time, mirroring how 'template_dir'
(singular), 'plugin_dir', and the other handlers already work.
Config values may be a native list (under config handlers with native
list support, e.g. ext_yaml or ext_json) or a comma-separated string
(required for the default ext_configparser / INI handler, since INI has
no native list syntax). The string form is split + whitespace-trimmed
via a parallel block to the existing 'extensions' handling, immediately
after the generic core_meta_override loop. Empty tokens (trailing or
repeated commas) are dropped — strip handles intra-token whitespace,
the empty filter handles missing tokens, both are required to avoid
landing '' as a resolved path.
The get/set in this block consistently use self._meta.config_section
(the same section the guard checks), so the normalization works
correctly when an app has explicitly customized Meta.config_section to
a value other than Meta.label. _setup_template_handler is left
untouched, so config-supplied dirs go through the same {label} /
{home_dir} substitution and dedup checks as Meta-supplied dirs and sit
in the same precedence slot:
core_user_template_dirs > template_dir > template_dirs > core_system_template_dirs
Resolves #746.
Quick task scaffolding for the PR #760 rewrite — PLAN.md captures the scope decisions (use core_meta_override, leave _setup_template_handler untouched, replace-not-extend semantics), and SUMMARY.md records the final state including verification results and the pre-push local-only status pending user review.
derks
force-pushed
the
feat/support-template-dirs-meta-override
branch
from
abc076c to
3167f38
Compare
2 participants
Add support for App.Meta.template_dirs and configuration override.
Resolves Issue #746
Summary by CodeRabbit
New Features
Documentation
Tests