snakemake/logging.py (1)

258-258: Normalize log level to lowercase in DefaultFormatter.format.

In the format method, level is not converted to lowercase before comparisons. This could lead to mismatches if the log level in msg is in uppercase. It's better to normalize level to lowercase for consistent handling.

Apply this change:

-def format(self, record):
     """
     Override format method to handle Snakemake-specific log messages.
     """
     msg = record.msg
-    level = msg.get("level", "INFO")
+    level = msg.get("level", "INFO").lower()

Configuration used: CodeRabbit UI
Review profile: CHILL

snakemake/logging.py (7)

246-509: LGTM! DefaultFormatter class is well-structured.

The DefaultFormatter class is well-organized and follows the single responsibility principle. It provides specific formatting for various log levels, which enhances readability and maintainability.

However, there are a few minor optimizations that could be made:

1. In the handle_info method, consider using a list comprehension for better performance:

output = [f"{indent}{line}" for line in msg["msg"].split("\n")]
return "\n".join(output)

2. In the format_percentage method, consider using f-strings for better readability:

fmt_precision = lambda p: f"{{:.{p}%}}"

These are minor suggestions and don't affect the overall functionality of the class.

---

587-635: LGTM! DefaultFilter class is well-implemented.

The DefaultFilter class is concise and effectively filters log messages based on quietness settings and the debug_dag flag. It makes good use of Snakemake's Quietness enum for filtering.

A minor optimization could be made in the filter method:

def filter(self, record):
    if self.is_quiet_about("all"):
        return False

    msg = record.msg
    level = msg.get("level", "INFO").lower()

    quietness_map = {
        "job_info": "rules",
        "group_info": "rules",
        "job_error": "rules",
        "group_error": "rules",
        "progress": "progress",
        "shellcmd": "progress",
        "job_finished": "progress",
        "resources_info": "progress",
        "run_info": "progress",
        "host": "host",
        "info": "progress",
    }

    if level in quietness_map and self.is_quiet_about(quietness_map[level]):
        return False

    if level == "dag_debug" and not self.debug_dag:
        return False

    return True

This change combines the check for level in quietness_map and the corresponding quietness check, slightly improving readability and performance.

🧰 Tools

🪛 Ruff

627-628: Use a single if statement instead of nested if statements

Combine if statements using and

(SIM102)

---

632-635: Return the condition not (level == "dag_debug" and not self.debug_dag) directly

Replace with return not (level == "dag_debug" and not self.debug_dag)

(SIM103)

---

Line range hint 737-920: LGTM! Logger class modifications enhance flexibility, but there's a potential issue with log level mapping.

The modifications to the Logger class improve its flexibility and customization options. The addition of default_formatter and default_filter attributes, along with the updated handler method, allows for more fine-grained control over logging behavior.

However, there's a potential issue in the handler method (lines 801-842):

log_level = {
    "debug": _logging.DEBUG,
    "info": _logging.WARNING,  # This mapping seems incorrect
    "warning": _logging.WARNING,
    "error": _logging.ERROR,
    "critical": _logging.CRITICAL,
    # ... other mappings ...
}.get(custom_level, _logging.INFO)

The "info" level is mapped to _logging.WARNING, which seems incorrect. Typically, "info" should be mapped to _logging.INFO. This mapping might cause "info" level messages to be treated as warnings, potentially leading to confusion or incorrect log filtering.

Consider updating this mapping to:

"info": _logging.INFO,

Unless there's a specific reason for this mapping that I'm not aware of, this change would align with standard logging practices.

---

Line range hint 923-979: LGTM! setup_logger function aligns well with the new logging structure, but contains a minor typo.

The modifications to the setup_logger function effectively integrate the new DefaultFormatter, DefaultFilter, and ColorizingTextHandler. This alignment with the new logging structure enhances consistency and maintainability.

However, there's a minor typo in the DefaultFormatter initialization (lines 961-965):

formatter = DefaultFormatter(
    printreason=printreason,
    show_failed_logs=show_failed_logs,
    printshellmds=printshellcmds,  # Typo here
)

The parameter printshellmds should be printshellcmds. This typo could lead to unexpected behavior if the printshellcmds setting is not properly applied.

Please correct the typo:

formatter = DefaultFormatter(
    printreason=printreason,
    show_failed_logs=show_failed_logs,
    printshellcmds=printshellcmds,
)

This correction ensures that the printshellcmds setting is properly passed to the DefaultFormatter.

---

513-513: Consider replacing lambda expressions with defined functions.

The static analysis tool suggests replacing lambda expressions with defined functions. This can improve readability and maintainability. For example, on lines 513 and 581:

fmt = lambda fraction: fmt_precision(precision).format(fraction)

Could be rewritten as:

def fmt(fraction):
    return fmt_precision(precision).format(fraction)

This change makes the code more explicit and easier to understand.

Also applies to: 581-581

🧰 Tools

🪛 Ruff

513-513: Do not assign a lambda expression, use a def

Rewrite fmt as a def

(E731)

---

523-523: Use a context manager for file operations.

On line 523, consider using a context manager (with statement) when opening files. This ensures that the file is properly closed after use, even if an exception occurs. For example:

with open(f, "r") as file:
    content = file.read()

This practice helps prevent resource leaks and is considered a Python best practice.

🧰 Tools

🪛 Ruff

523-523: Use a context manager for opening files

(SIM115)

---

537-537: Avoid ambiguous variable names.

On line 537, the variable name 'l' is used, which can be ambiguous (it looks like the number 1). Consider using a more descriptive variable name, such as 'line' or 'log_line'. This improves code readability.

🧰 Tools

🪛 Ruff

537-537: Ambiguous variable name: l

(E741)

Configuration used: CodeRabbit UI
Review profile: CHILL

snakemake/logging.py (3)

537-537: Rename ambiguous variable l to improve readability

The variable l can be confused with 1 or I. Rename it to a more descriptive name to enhance clarity.

Apply this diff:

-    max_len = min(max(max(len(l) for l in lines), len(logfile_header)), 80)
+    max_len = min(max(max(len(line) for line in lines), len(logfile_header)), 80)

🧰 Tools

🪛 Ruff

537-537: Ambiguous variable name: l

(E741)

---

678-678: Use != operator instead of not ... ==

Using != is more readable and idiomatic in Python.

Apply this diff:

-            return self.is_tty and not platform.system() == "Windows"
+            return self.is_tty and platform.system() != "Windows"

🧰 Tools

🪛 Ruff

678-678: Use platform.system() != "Windows" instead of not platform.system() == "Windows"

Replace with != operator

(SIM201)

---

713-713: Remove unused variable e

The variable e is assigned but never used. Removing it cleans up the code.

Apply this diff:

-                except Exception as e:
+                except Exception:

🧰 Tools

🪛 Ruff

713-713: Local variable e is assigned to but never used

Remove assignment to unused variable e

(F841)

Configuration used: CodeRabbit UI
Review profile: CHILL

snakemake/logging.py (2)

Line range hint 737-920: Improve Logger class implementation and documentation.

1. Consider moving the log level mapping dictionary in the handler method to a class attribute for better maintainability.

2. Add type hints to methods to improve code readability and maintainability.

3. Enhance the docstring for the logfile_hint method to be more descriptive.

Here's a suggestion for moving the log level mapping to a class attribute:

class Logger:
    LOG_LEVEL_MAPPING = {
        "debug": _logging.DEBUG,
        "info": _logging.WARNING,
        # ... other mappings ...
    }

    def handler(self, msg):
        custom_level = msg.get("level", "info").lower()
        log_level = self.LOG_LEVEL_MAPPING.get(custom_level, _logging.INFO)
        # ... rest of the method ...

Add type hints to methods, for example:

def set_stream_handler(self, stream_handler: _logging.Handler) -> None:
    # ... method implementation ...

def set_level(self, level: int) -> None:
    # ... method implementation ...

def handler(self, msg: dict) -> None:
    # ... method implementation ...

Enhance the logfile_hint method docstring:

def logfile_hint(self):
    """
    Log the location of the logfile if applicable.
    
    This method checks if the current execution mode is DEFAULT and not a dry run.
    If these conditions are met, it logs the relative path of the logfile.
    """
    # ... method implementation ...

These changes will improve the overall code quality and maintainability of the Logger class.

---

Line range hint 924-978: Improve setup_logger function implementation.

1. Add type hints to the function parameters and return value to improve code readability and maintainability.

2. Consider simplifying the quiet parameter handling.

3. The function is quite long and could potentially be split into smaller, more focused functions.

Here's a suggestion for adding type hints and simplifying the quiet parameter handling:

from typing import List, Optional, Set, Union
from snakemake.settings.types import Quietness
from snakemake_interface_executor_plugins.settings import ExecMode

def setup_logger(
    handler: List[_logging.Handler] = [],
    quiet: Union[bool, Set[Quietness], None] = False,
    printshellcmds: bool = False,
    printreason: bool = True,
    debug_dag: bool = False,
    nocolor: bool = False,
    stdout: bool = False,
    debug: bool = False,
    mode: Optional[ExecMode] = None,
    show_failed_logs: bool = False,
    dryrun: bool = False,
) -> None:
    if mode is None:
        mode = get_default_exec_mode()

    quiet_set = set()
    if isinstance(quiet, bool) and quiet:
        quiet_set = {Quietness.PROGRESS, Quietness.RULES}
    elif isinstance(quiet, set):
        quiet_set = quiet
    elif quiet is not None:
        raise ValueError("Unsupported value provided for quiet mode (either bool, None or set allowed).")

    # ... rest of the function ...

To further improve the function, consider splitting it into smaller, more focused functions:

def create_stream_handler(nocolor: bool, stdout: bool, mode: ExecMode) -> ColorizingTextHandler:
    # ... create and return stream handler ...

def create_formatter(printreason: bool, show_failed_logs: bool, printshellcmds: bool) -> DefaultFormatter:
    # ... create and return formatter ...

def create_filter(quiet: Set[Quietness], debug_dag: bool) -> DefaultFilter:
    # ... create and return filter ...

def setup_logger(...) -> None:
    # ... main setup logic, calling the above functions ...

These changes will improve the overall code quality, readability, and maintainability of the setup_logger function.

Configuration used: CodeRabbit UI
Review profile: CHILL

snakemake/logging.py (1)

401-438: LGTM: Well-structured _format_job_info method.

The _format_job_info method is well-implemented, handling various job information fields efficiently. The use of helper functions like format_item improves readability.

Consider using a dictionary for singleitems instead of conditionally appending 'reason'. This could make the code slightly more declarative:

singleitems = ["jobid", "benchmark"]
if self.printreason:
    singleitems.append("reason")

Could be refactored to:

singleitems = {
    "jobid": True,
    "benchmark": True,
    "reason": self.printreason
}

Then, you can iterate over the dictionary:

for item, include in singleitems.items():
    if include:
        fmt = format_item(item, omit=None)
        if fmt:
            output.append(fmt)

This is a minor suggestion and the current implementation is perfectly acceptable.

Configuration used: CodeRabbit UI
Review profile: CHILL