fix: Require emit() method implementation

[jlumpe]

Plugin handler classes will not work without an implementation of emit() (the logging.Handler version just raises NotImplementedError). The README mentions overriding the method but it is not obvious that it is a requirement.

This updates the example code in the README to include an emit() method. It also marks the method as abstract in LogHandlerBase. The latter is not strictly required but would probably result in a clearer error message, and the class already inherits from ABC.

Summary by CodeRabbit

• New Features

  • Standardized the log handler interface with a required logging-action hook, improving consistency; custom handlers may need updates.

• Documentation

  • Expanded examples showing filtering of error events, rich console output, formatted log data, and handling missing log files.
  • Clarified how to implement the required logging action in custom handlers.
  • Minor formatting and whitespace improvements to examples and comments.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds an abstract emit(record: LogRecord) method to LogHandlerBase and updates README examples to demonstrate overriding and using emit (including simple filtering and formatted output). Also contains minor whitespace and formatting tweaks in README examples.

Changes

Cohort / File(s)	Summary
Core API: Log handler base src/snakemake_interface_logger_plugins/base.py	Added abstract method emit(record: LogRecord) -> None to LogHandlerBase with docstring stating it performs the actual logging after the handler's filter.
Docs and examples README.md	Updated examples to show emit(self, record) override and usage (accessing/formatting LogRecord, filtering by event like JOB_ERROR, printing via rich), plus minor whitespace/formatting adjustments.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client as Snakemake Runtime
  participant Logger as Logger
  participant Handler as LogHandlerBase (subclass)

  Note over Client,Handler: Logging flow with new emit(record) contract

  Client->>Logger: log(LogRecord)
  Logger->>Handler: handle(record)
  alt passes Handler.filter(record)
    Handler->>Handler: emit(record)
    Note right of Handler: Subclass implements output (console/file/etc.)
  else filtered out
    Handler-->>Logger: no-op
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• feat: add OutputSettingsLoggerInterface and abstract methods to LogHandlerBase #11 — Modifies LogHandlerBase interface with additional abstract logging-related members; strongly related.
• feat!: API simplification #9 — Refactors LogHandlerBase inheritance/settings and removes create_handler; related to base class API changes.

Suggested reviewers

• johanneskoester

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title succinctly captures the core change of requiring an emit() method implementation, directly reflecting the PR’s update to mark emit() as abstract and clarify its necessity in the README.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a9b5230 and 3d73da9.

📒 Files selected for processing (1)

• src/snakemake_interface_logger_plugins/base.py (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/snakemake_interface_logger_plugins/base.py

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

• Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
• Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

README.md (2)

58-59: Avoid no-op attribute references in example

self.settings and self.common_settings as standalone expressions are no-ops and may confuse readers. Replace with a comment or remove.

-        # access settings attributes
-        self.settings
-        self.common_settings
+        # access settings via self.settings and self.common_settings as needed

---

157-164: Clarify rule-name filtering: prefer record.rule_name over record.name

In stdlib logging, record.name is the logger name, not the Snakemake rule name. If the rule name is exposed as record.rule_name, prefer that and fall back to record.name only if needed.

-            if hasattr(record, 'name') and record.name in ['rule1', 'rule2']:
+            rule_name = getattr(record, 'rule_name', getattr(record, 'name', None))
+            if rule_name in ['rule1', 'rule2']:

Can you confirm whether Snakemake consistently provides rule_name on JOB_ERROR records in the current version? If not, the fallback to record.name (logger name) should be explicitly documented as such to avoid confusion.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 42c55ff and 9167eb8.

📒 Files selected for processing (2)

• README.md (2 hunks)
• src/snakemake_interface_logger_plugins/base.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

• src/snakemake_interface_logger_plugins/base.py

🧠 Learnings (1)

📚 Learning: 2025-02-12T10:48:34.812Z

Learnt from: johanneskoester
PR: snakemake/snakemake-interface-logger-plugins#9
File: src/snakemake_interface_logger_plugins/registry/plugin.py:20-21
Timestamp: 2025-02-12T10:48:34.812Z
Learning: In the `Plugin` class of snakemake-interface-logger-plugins, the `log_handler` field should be annotated with `Type[LogHandlerBase]` as it stores the class type, not an instance.

Applied to files:

• src/snakemake_interface_logger_plugins/base.py

🔇 Additional comments (3)

src/snakemake_interface_logger_plugins/base.py (1)

30-36: Abstract emit() is a deliberate breaking change—confirm versioning/comms

Marking emit() as abstract will prevent instantiation of existing plugins that didn’t override it (now failing at import/instantiation rather than at first use). That’s preferable, but please ensure release notes and versioning reflect this requirement and that downstream templates/examples are updated (README has been updated—thanks).

README.md (2)

62-64: Good clarification that emit() is the only required Handler override

This sets the right expectation and points to the official docs. LGTM.

---

71-76: Emit() example is clear and actionable

The guidance to call self.format(record) and write to a stream/file is appropriate for plugin authors.

[cademirch]

thanks!

[cademirch]

Could you add the missing import? Happy to merge after tests pass.

[jlumpe]

Thanks for fixing, sorry wasn't able to get to it last night.