feat: support template_dirs config settings #760
Conversation
Resolves Issue #746
WalkthroughSupport for specifying Changes
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'
App->>App: Parse and set template_dirs from config
else Not found
App->>App: Use Meta.template_dirs
end
App->>TemplateHandler: Setup with final template_dirs list
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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.
| - `[core.foundation]` Support for App.Meta.template_dirs. | ||
| - [Issue #746](https://github.com/datafolklabs/cement/issues/749) |
There was a problem hiding this comment.
Fix incorrect issue reference.
The changelog entry references Issue #749, but according to the PR objectives, this feature addresses Issue #746.
- - [Issue #746](https://github.com/datafolklabs/cement/issues/749)
+ - [Issue #746](https://github.com/datafolklabs/cement/issues/746)📝 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.
| - `[core.foundation]` Support for App.Meta.template_dirs. | |
| - [Issue #746](https://github.com/datafolklabs/cement/issues/749) | |
| - `[core.foundation]` Support for App.Meta.template_dirs. | |
| - [Issue #746](https://github.com/datafolklabs/cement/issues/746) |
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 11 to 12, the issue reference is incorrect; it
currently points to Issue #749 instead of the correct Issue #746. Update the
changelog entry to reference Issue #746 to accurately reflect the PR objectives.
|
@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. |
Add support for App.Meta.template_dirs and configuration override.
Resolves Issue #746
Summary by CodeRabbit