Skip to content

Conversation

cademirch
Copy link
Collaborator

@cademirch cademirch commented Sep 28, 2025

This change was introduced in PR #35 but was not properly documented as breaking. Plugin authors need to ensure their LogHandlerBase implementations include an emit() method.

Summary by CodeRabbit

  • Tests
    • Added a reusable base class to standardize logger plugin tests (instantiation, required properties, and behavior checks).
    • Expanded coverage for stream/file exclusivity, emit callability, basic logging, and conditional file-writing.
    • Updated suite to exercise the real Rich log handler with a concrete settings stub.
    • Improved assertions and usage examples; prevented abstract base from being collected as tests.
    • Overall increases reliability and confidence in logger plugin implementations.

BREAKING CHANGE: All LogHandlerBase subclasses must now implement the emit() method.

  This change was introduced in PR #35 but was not properly documented as breaking.
  Plugin authors need to ensure their LogHandlerBase implementations include
  an emit() method that handles log record processing.
Copy link

coderabbitai bot commented Sep 28, 2025

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Adds a reusable test base for logger plugins (TestLogHandlerBase and MockOutputSettings) and updates project tests to use a concrete Rich log handler with corresponding mock settings. The new base provides standardized instantiation and capability checks; tests now validate against the real Rich handler via a concrete subclass.

Changes

Cohort / File(s) Summary
Logger plugin test scaffolding
src/snakemake_interface_logger_plugins/tests.py
Introduces MockOutputSettings and abstract TestLogHandlerBase with factory hooks, handler creation helper, and tests for instantiation, abstract properties, stream/file exclusivity, emit presence, basic logging, and conditional file-writing checks.
Concrete tests using Rich handler
tests/tests.py
Replaces mock plugin wiring with RichLogHandler and MockRichSettings. Adds TestConcreteRichPlugin implementing TestLogHandlerBase to run the base tests against Rich. Updates assertions and imports accordingly.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • johanneskoester
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch breaking-change-emit-method

📜 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 833394d and b4bcd73.

⛔ Files ignored due to path filters (1)
  • pyproject.toml is excluded by !pyproject.toml
📒 Files selected for processing (2)
  • src/snakemake_interface_logger_plugins/tests.py (1 hunks)
  • tests/tests.py (3 hunks)

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

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

@cademirch cademirch closed this Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant