feat: add logging#15
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughProvided concrete Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/tests.py (1)
1-83:⚠️ Potential issue | 🟠 MajorCI blocker: Black check is failing for this test file.
black --checkalready reports this file as non-compliant; please run Black to unblock CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tests.py` around lines 1 - 83, The test file containing TestRegistry, TestLogger, ReporterForTest, and the test_reporter_base_exposes_logger function is not Black-formatted; run Black (or apply Black formatting) on that test file to fix import spacing/linewrapping and any style issues, then commit the reformatted file so black --check passes in CI.snakemake_interface_report_plugins/reporter.py (1)
23-33:⚠️ Potential issue | 🟠 MajorPotentially breaking public interface change for third-party reporters.
The required
loggerparameter in__init__(Line 29) and new abstract methodget_exec_mode()(Lines 56-57) will break existingReporterBasesubclasses in external plugins that haven't been updated. Ensure release notes explicitly document this breaking change and provide migration guidance for downstream plugin maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakemake_interface_report_plugins/reporter.py` around lines 23 - 33, The change adds a required logger parameter to ReporterBase.__init__ and a new abstract method get_exec_mode(), which will break third-party ReporterBase subclasses; to make this backward-compatible, make the logger parameter optional (e.g., default to None) in __init__ and adapt any internal use to handle a missing logger, and convert get_exec_mode() from an abstract method to a concrete method on ReporterBase that returns a sensible default exec mode (or provide a protected compatibility shim like _get_exec_mode() that calls get_exec_mode() if overridden), while documenting the breaking change and migration steps in the release notes for downstream plugin maintainers.
🧹 Nitpick comments (1)
tests/tests.py (1)
70-81: Use a minimal DAG stub instead ofNonefor stronger contract coverage.Passing
dag=Noneon Line 80 bypasses theDAGReportInterfacecontract in this constructor test. A tiny stub object would make the test more robust against futureReporterBasechanges.Suggested test hardening
+class DummyDag: + def toposorted(self): + return [] + + `@property` + def dependencies(self): + return {} + def test_reporter_base_exposes_logger(): logger = TestLogger() reporter = ReporterForTest( rules={}, results={}, logger=logger, configfiles=[], jobs=[], settings=ReportSettingsBase(), workflow_description="", - dag=None, + dag=DummyDag(), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tests.py` around lines 70 - 81, Replace the sentinel None passed as dag in test_reporter_base_exposes_logger with a minimal DAG stub that implements the DAGReportInterface used by ReporterForTest so the constructor contract is exercised; update the test to instantiate a tiny object (e.g., class or simple dataclass) that provides the minimal attributes/methods the ReporterBase expects and pass that instance to ReporterForTest instead of None to strengthen contract coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@snakemake_interface_report_plugins/reporter.py`:
- Around line 23-33: The change adds a required logger parameter to
ReporterBase.__init__ and a new abstract method get_exec_mode(), which will
break third-party ReporterBase subclasses; to make this backward-compatible,
make the logger parameter optional (e.g., default to None) in __init__ and adapt
any internal use to handle a missing logger, and convert get_exec_mode() from an
abstract method to a concrete method on ReporterBase that returns a sensible
default exec mode (or provide a protected compatibility shim like
_get_exec_mode() that calls get_exec_mode() if overridden), while documenting
the breaking change and migration steps in the release notes for downstream
plugin maintainers.
In `@tests/tests.py`:
- Around line 1-83: The test file containing TestRegistry, TestLogger,
ReporterForTest, and the test_reporter_base_exposes_logger function is not
Black-formatted; run Black (or apply Black formatting) on that test file to fix
import spacing/linewrapping and any style issues, then commit the reformatted
file so black --check passes in CI.
---
Nitpick comments:
In `@tests/tests.py`:
- Around line 70-81: Replace the sentinel None passed as dag in
test_reporter_base_exposes_logger with a minimal DAG stub that implements the
DAGReportInterface used by ReporterForTest so the constructor contract is
exercised; update the test to instantiate a tiny object (e.g., class or simple
dataclass) that provides the minimal attributes/methods the ReporterBase expects
and pass that instance to ReporterForTest instead of None to strengthen contract
coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d282bad-bd1b-498d-8828-88c34d5bfe63
📒 Files selected for processing (5)
snakemake_interface_report_plugins/interfaces.pysnakemake_interface_report_plugins/logging.pysnakemake_interface_report_plugins/reporter.pysnakemake_interface_report_plugins/settings.pytests/tests.py
|
|
||
| @abstractmethod | ||
| def render(self): ... | ||
| def get_exec_mode(self) -> ExecMode: |
There was a problem hiding this comment.
This I don't understand. How should the reporter implement that?
There was a problem hiding this comment.
I thought it to be a formal requirement / necessity as the executor base classes in the interface module define this as well. So, this line stems from my template the executor interface plugin.
| from abc import ABC, abstractmethod | ||
|
|
||
|
|
||
| class LoggingReportInterface(ABC): |
There was a problem hiding this comment.
Is this really needed, I think the logger is just a standard python logger nowadays, isn't it?
There was a problem hiding this comment.
no idea - honestly, I copied from the executor interface, and it defines the logger in the ReporterBase class. (Ok, the executor interface speaks of LoggerExecutorInterace in place of Logging...).
So, how can this functionality be inherited if not by defining this explicitly?
| def software(self) -> List[SoftwareReport]: | ||
| return [] | ||
|
|
There was a problem hiding this comment.
well spotted. I introduced this, experimentally, but don't need it anymore.
2 participants
This interface needs support to inherit the Snakemake logger interface to report results of a report on the command line (or issue warnings ...)
Context:
The nanopub report plugin is the first plugin in the official repo base to use this interface. Upon publication of a nanopub, the nanopub URI is returned. It should be reported or - in case of an error - the error message. (Eventually, the plugin will support retracting faulty nanopubs, too. But this feature is not stable in the nanopub-py library.)
Changes made in the report interface plugin
LoggingReportInterfaceJobRecordInterface- and allowing the software report to be imported and usedObserved outcome
The new plugin will be tested extensively before publication of a release, yet this interface now supports issuing all necessary message.
Summary by CodeRabbit
New Features
Refactor
Tests