fix: Add type annotations in logging module#3757
fix: Add type annotations in logging module#3757johanneskoester merged 6 commits intosnakemake:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds comprehensive type annotations and public attribute declarations to logging components in src/snakemake/logging.py. Function and method signatures now use explicit types (including Optional and Collection) and concrete return types; constructors and helper methods for formatters, filters, handlers, and LoggerManager are retyped. No behavioral changes indicated. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/snakemake/logging.py (3)
90-106: Guard against division by zero in format_percentage()total can be 0 (e.g., empty DAG), which raises ZeroDivisionError. Add a fast path for non-positive totals.
Apply this diff:
def format_percentage(done: int, total: int) -> str: """Format percentage from given fraction while avoiding superfluous precision.""" + # Avoid division by zero / invalid totals + if total <= 0: + return "0%" if done == total: return "100%" if done == 0: return "0%"
108-121: Docstring return type mismatch in get_event_level()Annotation is tuple[Optional[LogEvent], str] but docstring says tuple[LogEvent, str]. Align the docstring.
Apply this diff:
- Returns: - tuple[LogEvent, str] + Returns: + tuple[Optional[LogEvent], str]
390-399: Dry-run filter condition never matcheslevel is record.levelname (e.g., "INFO"), so comparing to "run_info" is always false; run-info messages won’t be allowed through during dry runs as intended. Compare the event instead.
Apply this diff:
- if self.dryrun and level == "run_info": + if self.dryrun and event == LogEvent.RUN_INFO: return True
🧹 Nitpick comments (6)
src/snakemake/logging.py (6)
509-512: Ensure is_tty returns a real boolCurrently returns None when stream lacks isatty, violating the -> bool contract (and confusing type checkers). Coerce to bool.
Apply this diff:
@property def is_tty(self) -> bool: isatty = getattr(self.stream, "isatty", None) - return isatty and isatty() + return bool(isatty and isatty())
647-654: Assert settings is initialized where requiredThese helpers assume settings is set by setup(); add an assert to placate static checkers and fail fast if misused.
Apply this diff:
def _default_filter(self) -> DefaultFilter: + assert self.settings is not None, "LoggerManager.settings not initialized; call setup() first." return DefaultFilter( self.settings.quiet, self.settings.debug_dag, self.settings.dryrun, self.settings.printshellcmds, )
655-660: Same here: guard self.settings in _default_formatter()Apply this diff:
def _default_formatter(self) -> DefaultFormatter: + assert self.settings is not None, "LoggerManager.settings not initialized; call setup() first." return DefaultFormatter( self.settings.quiet, self.settings.show_failed_logs, )
661-669: And in _default_filehandler(): guard settings before useApply this diff:
def _default_filehandler(self, logfile) -> logging.Handler: + assert self.settings is not None, "LoggerManager.settings not initialized; call setup() first." logfile_handler = logging.FileHandler(logfile) logfile_handler.setFormatter(self._default_formatter()) logfile_handler.addFilter(self._default_filter()) logfile_handler.setLevel( logging.DEBUG if self.settings.verbose else logging.INFO )
671-680: And in _default_streamhandler(): guard settings before useApply this diff:
def _default_streamhandler(self) -> logging.Handler: + assert self.settings is not None, "LoggerManager.settings not initialized; call setup() first." stream_handler = ColorizingTextHandler( nocolor=self.settings.nocolor, stream=sys.stdout if self.settings.stdout else sys.stderr, mode=self.mode, )
723-726: Avoid touching private attribute _thread in stop()Relying on a private attribute is brittle; QueueListener.stop() is safe after start(). Check for None only.
Apply this diff:
def stop(self) -> None: - if self.queue_listener is not None and self.queue_listener._thread is not None: - self.queue_listener.stop() + if self.queue_listener is not None: + self.queue_listener.stop()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/snakemake/logging.py(16 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:
src/snakemake/logging.py
🔇 Additional comments (3)
src/snakemake/logging.py (3)
32-35: LGTM: precise return type for timestamp()Clear and correct.
123-129: LGTM: is_quiet_about() signature and logicAccepting Collection[Quietness] and parsing msg_type is appropriate.
132-141: LGTM: DefaultFormatter attribute typing and constructorOptional quiet handled correctly with set() default.
cademirch
changed the title
cademirch
left a comment
cademirch
left a comment
There was a problem hiding this comment.
Thanks for adding the type hints! If you could add the one coderabbit suggestion, as well as handle the merge conflicts that would be great!
One more potential issue that CodeRabbit pointed out: in DefaultFilter.filter(), record.levelname is checked for equality against "run_info" when it should probably be checking record.event. |
Great! Thanks.
I'm inclined to agree, but am curious what (if any) change in behavior this will result in. |
cademirch
left a comment
cademirch
left a comment
There was a problem hiding this comment.
Please fix formatting, then ready to merge as long as tests pass!
|
Formatting fixed. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/snakemake/logging.py (1)
380-421: Critical bug: incorrect comparison at line 384.Line 384 compares
level(which isrecord.levelname) to the string"run_info":if self.dryrun and level == "run_info":However,
record.levelnamecontains standard Python logging levels (INFO,DEBUG,WARNING,ERROR,CRITICAL) and will never equal"run_info". This condition will never be True, breaking the intended dryrun filtering logic.Fix: Compare
eventtoLogEvent.RUN_INFOinstead:if self.dryrun and event == LogEvent.RUN_INFO:This matches the pattern used elsewhere in the codebase (e.g., line 535:
if level == "INFO" and event in self.yellow_info_events:).
🧹 Nitpick comments (1)
src/snakemake/logging.py (1)
563-587: Consider adding explicit return type annotation.The parameter type annotations are correct, but the method contains
returnstatements (line 587) and could benefit from an explicit-> Nonereturn type annotation for completeness.Apply this diff:
def setup( self, handlers: List[LogHandlerBase], settings: OutputSettingsLoggerInterface, - ): + ) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/snakemake/logging.py(14 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:
src/snakemake/logging.py
🪛 Ruff (0.13.1)
src/snakemake/logging.py
451-451: Undefined name ExecMode
(F821)
550-550: Undefined name ExecMode
(F821)
🔇 Additional comments (20)
src/snakemake/logging.py (20)
22-22: LGTM!The addition of
CollectionandTextIOto the typing imports is appropriate and necessary for the type annotations introduced throughout the module.
31-33: LGTM!The return type annotation
-> strcorrectly reflects thattimestamp()returns a formatted string.
89-104: LGTM!The parameter and return type annotations are accurate for this percentage formatting function.
107-110: LGTM!The updated signature and simplified docstring are improvements. Moving type information from docstring to the signature aligns with modern Python typing practices.
113-118: LGTM!Broadening the
quietparameter fromQuietnesstoCollection["Quietness"]correctly reflects that the function accepts any collection of quietness values.
121-132: LGTM!The class attribute declarations and constructor signature correctly document the public API. The use of
Optional[Collection["Quietness"]]with a None-safe default is appropriate.
134-158: LGTM!The
formatmethod's type annotation for therecordparameter is correct.
360-379: LGTM!The class attribute declarations and constructor signature correctly document the DefaultFilter API with appropriate typing.
490-492: LGTM!Explicitly returning
boolfromis_ttyresolves the previous review concern and ensures the property always returns a boolean type.
494-524: LGTM!The type annotation for the
recordparameter in theemitmethod is correct.
526-543: LGTM!The type annotations for both parameters in the
decoratemethod are appropriate.
547-562: LGTM with one caveat.The class attribute declarations correctly document the LoggerManager's public API. The use of
Optionalis appropriate since these attributes areNonebeforesetup()is called. The correction oflogfile_handlerstodict[logging.Handler, str](from the narrowerdict[LogHandlerBase, str]) properly accommodates both plugin handlers and standardFileHandlerinstances.Note: The
modeattribute at line 550 has the sameExecModeundefined issue flagged inColorizingTextHandler.
634-639: LGTM!The return type annotation correctly reflects that this method returns the plugin handler it configured.
641-647: LGTM!The return type annotation is specific and accurate.
649-653: LGTM!The return type annotation is specific and accurate.
655-663: LGTM!The return type
logging.Handleris appropriate for this factory method that creates aFileHandler.
665-673: LGTM!The return type
logging.Handleris appropriate for this factory method that creates aColorizingTextHandler.
686-690: LGTM!The return type annotation
-> Noneis correct for this cleanup method.
692-712: LGTM!The parameter and return type annotations are appropriate. The use of
Optional[os.PathLike]correctly indicates thatworkdircan beNone.
714-716: LGTM!The return type annotation
-> Noneis correct for this teardown method.
|
Aaand fixed the remaining |
🤖 I have created a release *beep* *boop* --- ## [9.11.7](v9.11.6...v9.11.7) (2025-10-01) ### Bug Fixes * Add type annotations in logging module ([#3757](#3757)) ([63404b2](63404b2)) * source_path not mounted ([#3738](#3738)) ([86e30b3](86e30b3)) ### Documentation * mastodon announcements ([#3723](#3723)) ([7da1b18](7da1b18)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Was trying to understand the logging system better and ended up filling in most of the missing type annotations. These were mostly pretty clear, but there are a few potential issues: - The first argument of `is_quiet_about()` and the `quiet` attribute of `DefaultFormatter` and `DefaultFilter` were annotated as `Quietness`, which does not seem correct. I have changed these to `Container[Quietness]`. - `LoggerManager` is a bit of a difficult case because a lot of attributes are `None` before `setup()` is called. I have annotated these with `Optional`, but it might make more sense to do the typing as if `setup()` had already been called, even if it's not technically correct just after construction. Also corrected the docstring of `get_event_level()`. ### QC <!-- Make sure that you can tick the boxes below. --> * [x] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [x] The documentation (`docs/`) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake). I ran the test suite applying [typeguard](https://typeguard.readthedocs.io) to the logging module. Nearly every function with added/changed annotations was covered. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Improved logging output: clearer messages, enhanced color/TTY handling, and finer control over quietness and failed-log visibility. - Refactor - Standardized and strengthened public logging interfaces, handlers, formatters, and filters for more predictable terminal and file output. - Documentation - Updated logging docs to reflect clarified behaviors and configuration options. - Chores - Broad typing and public API alignment to improve maintainability without changing user-facing behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 I have created a release *beep* *boop* --- ## [9.11.7](snakemake/snakemake@v9.11.6...v9.11.7) (2025-10-01) ### Bug Fixes * Add type annotations in logging module ([snakemake#3757](snakemake#3757)) ([63404b2](snakemake@63404b2)) * source_path not mounted ([snakemake#3738](snakemake#3738)) ([86e30b3](snakemake@86e30b3)) ### Documentation * mastodon announcements ([snakemake#3723](snakemake#3723)) ([7da1b18](snakemake@7da1b18)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
3 participants
Was trying to understand the logging system better and ended up filling in most of the missing type annotations. These were mostly pretty clear, but there are a few potential issues:
is_quiet_about()and thequietattribute ofDefaultFormatterandDefaultFilterwere annotated asQuietness, which does not seem correct. I have changed these toContainer[Quietness].LoggerManageris a bit of a difficult case because a lot of attributes areNonebeforesetup()is called. I have annotated these withOptional, but it might make more sense to do the typing as ifsetup()had already been called, even if it's not technically correct just after construction.Also corrected the docstring of
get_event_level().QC
docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).I ran the test suite applying typeguard to the logging module. Nearly every function with added/changed annotations was covered.
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores