feat: add logger plugin scaffold #94
Conversation
📝 WalkthroughWalkthroughAdds scaffolding support for a Snakemake logger plugin: new scaffold plugin class, logger plugin template (module + tests), package registration, and inclusion of Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as snakedeploy CLI
participant Scaffold as ScaffoldSnakemakeLoggerPlugin
participant Templates as logger-plugins templates
participant FS as Filesystem
CLI->>Scaffold: request scaffold type="logger"
activate Scaffold
Scaffold->>Templates: get_templates(module_path, tests_path)
Templates-->>Scaffold: list of (template_name, target_path)
Scaffold->>FS: write module and test files to target paths
FS-->>Scaffold: write results
Scaffold-->>CLI: return generated paths / status
deactivate Scaffold
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
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. Tip 🧪 Early access (models): enabledWe are currently testing Sonnet 4.5 code review models, which should lead to better review quality. However, this model may result in higher noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 0
🧹 Nitpick comments (1)
snakedeploy/templates/plugins/logger-plugins/init.py (1)
8-32: Fix placeholder values in metadata.The dataclass structure and documentation are excellent for a template. However, the ellipsis placeholders in the metadata should be handled properly.
# Optionally specify a function that parses the value given by the user. # This is useful to create complex types from the user input. - "parse_func": ..., + # "parse_func": your_parse_function, # If a parse_func is specified, you also have to specify an unparse_func # that converts the parsed value back to a string. - "unparse_func": ..., + # "unparse_func": your_unparse_function,This prevents potential syntax errors when developers use the template and provides clearer guidance on how to specify these optional functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snakedeploy/scaffold_plugins/logger_plugins.py(1 hunks)snakedeploy/templates/plugins/logger-plugins/init.py(1 hunks)tests/test_client.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit Configuration File
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
snakedeploy/scaffold_plugins/logger_plugins.pysnakedeploy/templates/plugins/logger-plugins/init.py
🧬 Code Graph Analysis (1)
snakedeploy/scaffold_plugins/logger_plugins.py (2)
snakedeploy/scaffold_plugins/common.py (1)
ScaffoldPlugin(11-170)snakedeploy/providers.py (1)
name(87-88)
🔇 Additional comments (9)
snakedeploy/scaffold_plugins/logger_plugins.py (4)
7-12: LGTM!The class definition follows the proper inheritance pattern and includes appropriate name and description attributes that clearly identify the plugin's purpose.
14-20: LGTM!The method correctly implements the abstract interface, providing appropriate template mappings for the logger plugin scaffolding.
22-23: LGTM!The method correctly returns "logger" as the plugin type, which aligns with the scaffolding purpose.
25-26: Logger plugins correctly opt-out of Snakemake dev dependencyThe
include_snakemake_dev_dependency()override inlogger_plugins.pyreturnsFalse, matching the behavior of storage, software-deployment, and scheduler plugins. Only executor and report plugins returnTrue(to pull in Snakemake for their specific test scenarios). No change needed here.tests/test_client.sh (1)
88-88: LGTM!Properly adds logger plugin type to the automated scaffolding tests, ensuring the new functionality is covered by the test suite.
snakedeploy/templates/plugins/logger-plugins/init.py (4)
1-6: LGTM!The imports are well-organized and include the necessary dependencies for creating a logger plugin template.
35-46: LGTM!Excellent template structure with comprehensive documentation. The use of
__post_init__instead of overriding__init__is well-explained and follows the plugin interface best practices.
48-56: LGTM!Excellent educational comments that provide valuable context about Snakemake's logging system and point developers to relevant documentation and examples.
58-93: LGTM!The property methods are well-documented with comprehensive guidance for developers. The placeholder implementations with ellipsis are appropriate for a template, and the extensive comments clearly explain the purpose and expected behavior of each property.
|
I agree, not straightforward what the test class should test for loggers. Maybe it is more like testing instantiation of the logger, giving it some dummy messages and just expecting them to be processed without error. That should be enough as a start. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
snakedeploy/scaffold_plugins/__init__.py (1)
1-31: Address Ruff TRY003 violation
Insnakedeploy/scaffold_plugins/__init__.py:31, avoid specifying long messages outside exception classes—move the “Unknown plugin type: {plugin_type}” message intoUserErroror simplify theraise.
Runruff check --exit-zeroto confirm no remaining violations.
🧹 Nitpick comments (1)
snakedeploy/templates/plugins/logger-plugins/tests.py (1)
1-1: Make the template test actionable and generic (remove “rich”, implement hooks).Replace ellipses with a minimal, working implementation that imports the scaffolded plugin, with an env override for flexibility. Also generalize the wording.
-from snakemake_interface_logger_plugins.tests import TestLogHandlerBase +from snakemake_interface_logger_plugins.tests import TestLogHandlerBase +import importlib +import os @@ -class TestConcreteRichPlugin(TestLogHandlerBase): - """Concrete test using the actual rich plugin to verify the abstract test class works.""" +class TestTemplateLoggerPlugin(TestLogHandlerBase): + """Concrete test using the scaffolded logger plugin to verify the abstract test base.""" @@ - def get_log_handler_cls(self): - """Return the rich log handler class.""" - ... + def get_log_handler_cls(self): + """Return the scaffolded LogHandler class.""" + module_name = os.environ.get("SNAKEPLUGIN_MODULE", "your_plugin_package") + module = importlib.import_module(module_name) + return module.LogHandler @@ - def get_log_handler_settings(self): - """Return the rich settings with default values for testing.""" - ... + def get_log_handler_settings(self): + """Return default scaffolded LogHandlerSettings for testing.""" + module_name = os.environ.get("SNAKEPLUGIN_MODULE", "your_plugin_package") + module = importlib.import_module(module_name) + return module.LogHandlerSettings()Note: replace "your_plugin_package" with the actual package name in the scaffold, or set SNAKEPLUGIN_MODULE in CI.
Also applies to: 4-16
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakedeploy/scaffold_plugins/__init__.py(2 hunks)snakedeploy/templates/plugins/logger-plugins/tests.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
snakedeploy/scaffold_plugins/__init__.pysnakedeploy/templates/plugins/logger-plugins/tests.py
🧬 Code graph analysis (1)
snakedeploy/scaffold_plugins/__init__.py (1)
snakedeploy/scaffold_plugins/logger_plugins.py (1)
ScaffoldSnakemakeLoggerPlugin(7-26)
🪛 GitHub Actions: CI
snakedeploy/scaffold_plugins/__init__.py
[error] 1-1: Ruff formatting check detected that the file would be reformatted by 'ruff format --check'. Run 'ruff format' to fix code style issues.
🔇 Additional comments (1)
snakedeploy/scaffold_plugins/__init__.py (1)
13-13: Logger plugin dispatch: LGTM; scaffolding already sets project.requires-python to>=3.11,<4.0(common.py:66), which satisfies thematch/caserequirement.
|
Unsure about why test is failing seems related to the workflow template? |
mpusp workflow template switched from config.yml to config.yaml
🤖 I have created a release *beep* *boop* --- ## [0.13.0](v0.12.3...v0.13.0) (2025-09-29) ### Features * add logger plugin scaffold ([#94](#94)) ([da2d149](da2d149)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Adds scaffolding for logger plugin.
tests.py is incomplete. It was rather difficult for me to come up with a way to build an abstract test class for logging.
I considered using https://github.com/snakemake/snakemake/blob/main/src/snakemake/common/tests/__init__.py like the reporter does, but it would take a fair amount of changes in that code to accomodate specifiying the logger plugin.
Testing logging is also a bit difficult in general since for the most part it requires visual inspection.
I can try to find some time to tinker with the tests more, but would appreciate any feedback.
Summary by CodeRabbit