feat: add OutputSettingsLoggerInterface and abstract methods to LogHandlerBase #11
Conversation
📝 WalkthroughWalkthroughThe changes involve modifications to the Changes
Possibly related PRs
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
…thods to communicate log plugin behavior new argument to LogHandlerBase init
cademirch
changed the title
![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 (2)
src/snakemake_interface_logger_plugins/base.py (1)
18-25: Constructor logic is cohesive.
Storingcommon_settingsand immediately checking for conflicting I/O modes (stream vs. file) is a good design choice. Consider clarifying the error message to guide developers on valid usage or referencing documentation about valid configurations.src/snakemake_interface_logger_plugins/settings.py (1)
15-23: Interface attributes lacking default values.
Defining these as bare attributes in an ABC is valid. If you need to enforce initialization or specific default behaviors in child classes, consider making them abstract properties or providing constructors in subclasses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/snakemake_interface_logger_plugins/base.py(1 hunks)src/snakemake_interface_logger_plugins/registry/__init__.py(2 hunks)src/snakemake_interface_logger_plugins/settings.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/snakemake_interface_logger_plugins/registry/init.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest ...
**/*.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.
src/snakemake_interface_logger_plugins/base.pysrc/snakemake_interface_logger_plugins/settings.py
🪛 GitHub Actions: CI
src/snakemake_interface_logger_plugins/base.py
[error] 1-1: Ruff formatting check failed. Run 'ruff format' to fix code style issues in this file.
🔇 Additional comments (9)
src/snakemake_interface_logger_plugins/base.py (8)
1-1: Address the Ruff formatting error.
The pipeline indicates that a Ruff formatting check has failed. Please runruff format(or your preferred formatting command) to ensure the file meets the project's code style standards.🧰 Tools
🪛 Ruff (0.8.2)
1-1: No such file or directory (os error 2)
(E902)
🪛 GitHub Actions: CI
[error] 1-1: Ruff formatting check failed. Run 'ruff format' to fix code style issues in this file.
7-11: Imports look correct.
These imports align well with the usage below and introduce the necessary base interfaces and methods.
15-15: Marking the class as an abstract base class is appropriate.
MakingLogHandlerBasederive fromABCclarifies that subclasses must implement the required abstract methods and properties.
27-27: Utility method for post-initialization.
__post__init__provides flexibility for subclasses to extend setup logic. Good forward-thinking design.
30-35: Abstract property for stream-writing logic is well-defined.
Ensuring each subclass implements whether it writes to stdout/stderr fosters clarity in the logging pipeline.
37-42: Abstract property for file-writing is well-defined.
Similar towrites_to_stream, this property clarifies log file handling requirements for subclasses.
44-49: Filtering responsibility placed on subclasses.
Thehas_filterproperty is a clean way to indicate whether a subclass is responsible for its own log filtering.
51-56: Formatter presence determined by subclass.
Thehas_formatterproperty neatly separates concerns, signaling that subclasses can specify custom formatting responsibilities.src/snakemake_interface_logger_plugins/settings.py (1)
11-13: Additional imports are appropriate.
TheABCimport aligns with the creation of an abstract interface, and usingOptionalandSequenceis valid for these attributes.
|
Hi @johanneskoester I've made some changes here to align with changes made in snakemake/snakemake#3107. I think its worth giving the LogHandlerBase access to Snakemake's OutputSettings, so that LogHandlerPlugins can define their own formatters and filters. Thus I added We could just make a I've also added some abstract methods to |
renamed plugin attributes and settings interface
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/snakemake_interface_logger_plugins/base.py (1)
27-28: Document the purpose of the empty post-init method.Consider adding a docstring to explain why this method exists and when subclasses should override it.
def __post__init__(self) -> None: + """Hook for subclasses to perform additional initialization after __init__. + + This method is called at the end of __init__ and can be overridden by subclasses + to perform any necessary setup without having to override __init__. + """ pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/snakemake_interface_logger_plugins/base.py(1 hunks)src/snakemake_interface_logger_plugins/registry/__init__.py(3 hunks)src/snakemake_interface_logger_plugins/settings.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/snakemake_interface_logger_plugins/registry/init.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest ...
**/*.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.
src/snakemake_interface_logger_plugins/settings.pysrc/snakemake_interface_logger_plugins/base.py
🔇 Additional comments (3)
src/snakemake_interface_logger_plugins/settings.py (1)
15-24: LGTM!The interface is well-structured with appropriate type annotations for all attributes. The class follows the naming convention and correctly inherits from ABC.
src/snakemake_interface_logger_plugins/base.py (2)
15-26: LGTM!The class is well-structured with appropriate inheritance and constructor implementation. The validation check for mutually exclusive behaviors (
writes_to_streamandwrites_to_file) is a good safeguard.
30-56: LGTM!The abstract properties are well-defined with clear docstrings and appropriate return type annotations. They effectively communicate the expected behavior of logger plugins.
Thanks, fixed! And will do - sorry about that. |
johanneskoester
changed the title
Summary by CodeRabbit
New Features
Refactor