Skip to content

Logger with file #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sahiljhawar merged 3 commits into from
Feb 13, 2026
Merged

Logger with file #54

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2476aec
refactor: use API to fetch Hp index
sahiljhawar Feb 13, 2026
8e55164
fix(logger): use a root logger and log_file in setup_logging
sahiljhawar Feb 13, 2026
6198ab0
Merge branch 'main' into logger-with-file
sahiljhawar Feb 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 38 additions & 14 deletions swvo/logger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@

import logging
import sys
from pathlib import Path
from typing import Optional

logger = logging.getLogger(__name__)

# Get the package logger
logger = logging.getLogger("swvo")
logger.addHandler(logging.NullHandler())

Expand All @@ -28,16 +29,39 @@ def format(self, record):
return f"{color}{msg}{self.RESET}"


def setup_logging(level=logging.INFO):
if any(isinstance(h, logging.StreamHandler) for h in logger.handlers):
def setup_logging(level: str = "INFO", log_file: Optional[Path] = None, file_mode: str = "w"):
"""Setup logging for the swvo package and root logger.

Parameters
----------
level : str, optional
Logging level, by default is INFO
log_file : Path, optional
Path to log file. If None, only console logging is enabled.If provided, logs will be written to both console and file., by default None

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space after period in the docstring. The text "enabled.If" should be "enabled. If" with a space separating the two sentences.

Suggested change
Path to log file. If None, only console logging is enabled.If provided, logs will be written to both console and file., by default None
Path to log file. If None, only console logging is enabled. If provided, logs will be written to both console and file., by default None

Copilot uses AI. Check for mistakes.
Comment on lines +38 to +40

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring is missing documentation for the file_mode parameter. All parameters should be documented in the Parameters section, including file_mode which controls whether the log file is overwritten ("w") or appended to ("a").

Suggested change
Logging level, by default is INFO
log_file : Path, optional
Path to log file. If None, only console logging is enabled.If provided, logs will be written to both console and file., by default None
Logging level, by default is INFO.
log_file : Path, optional
Path to log file. If None, only console logging is enabled. If provided,
logs will be written to both console and file. By default None.
file_mode : str, optional
File open mode for the log file: ``"w"`` to overwrite or ``"a"`` to append.
By default ``"w"``.

Copilot uses AI. Check for mistakes.
"""
try:
level = getattr(logging, level.upper())
except AttributeError:
raise ValueError(f"Invalid logging level: {level}. Use one of DEBUG, INFO, WARNING, ERROR, CRITICAL.")
Comment on lines +32 to +45

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function signature has changed from accepting a logging level constant (e.g., logging.INFO) to accepting a string (e.g., "INFO"). This is a breaking change that will cause the existing call setup_logging() in download_daily_files.py to continue working with the default, but any existing code that passes a logging level constant like setup_logging(logging.DEBUG) will fail. The old signature was level=logging.INFO (an int constant), and the new signature is level: str = "INFO" (a string).

Suggested change
def setup_logging(level: str = "INFO", log_file: Optional[Path] = None, file_mode: str = "w"):
"""Setup logging for the swvo package and root logger.
Parameters
----------
level : str, optional
Logging level, by default is INFO
log_file : Path, optional
Path to log file. If None, only console logging is enabled.If provided, logs will be written to both console and file., by default None
"""
try:
level = getattr(logging, level.upper())
except AttributeError:
raise ValueError(f"Invalid logging level: {level}. Use one of DEBUG, INFO, WARNING, ERROR, CRITICAL.")
def setup_logging(level=logging.INFO, log_file: Optional[Path] = None, file_mode: str = "w"):
"""Setup logging for the swvo package and root logger.
Parameters
----------
level : int or str, optional
Logging level. Can be a logging level constant (e.g., logging.INFO) or a string
(e.g., "INFO"). By default is INFO.
log_file : Path, optional
Path to log file. If None, only console logging is enabled.If provided, logs will be written to both console and file., by default None
"""
if isinstance(level, str):
try:
level = getattr(logging, level.upper())
except AttributeError:
raise ValueError(
f"Invalid logging level: {level}. Use one of DEBUG, INFO, WARNING, ERROR, CRITICAL."
)
elif not isinstance(level, int):
raise ValueError(
f"Invalid logging level type: {type(level)!r}. Expected int or str."
)

Copilot uses AI. Check for mistakes.

# Configure root logger so all loggers inherit the formatting
root_logger = logging.getLogger()

# Check if already configured
if any(isinstance(h, logging.StreamHandler) for h in root_logger.handlers):
return
Comment on lines +51 to 52

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check for existing configuration now looks at the root logger instead of the package logger. This changes the semantics of the function - it will now skip configuration if ANY StreamHandler exists on the root logger, even if it wasn't added by this function. This could prevent proper setup if another library has already configured the root logger. The previous implementation checked logger.handlers (the "swvo" package logger), which was more isolated.

Copilot uses AI. Check for mistakes.
handler = logging.StreamHandler(sys.stdout)
handler.setFormatter(
_ColorFormatter(
"[%(levelname)-8s] %(asctime)s - %(name)s - %(message)s",
datefmt="%Y-%m-%d %H:%M:%S",
)
)

logger.setLevel(level)
logger.addHandler(handler)

# Console handler with colors
log_format = "[%(levelname)-8s] %(asctime)s - %(name)s:%(lineno)d - %(message)s"

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log format has been changed to include line numbers (:%(lineno)d). While this can be useful for debugging, it represents a breaking change in the log output format that may affect any systems or scripts that parse the log files. This change should be documented in the PR description or changelog.

Suggested change
log_format = "[%(levelname)-8s] %(asctime)s - %(name)s:%(lineno)d - %(message)s"
log_format = "[%(levelname)-8s] %(asctime)s - %(name)s - %(message)s"

Copilot uses AI. Check for mistakes.
datefmt = "%Y-%m-%d %H:%M:%S"

console_handler = logging.StreamHandler(sys.stdout)
console_handler.setFormatter(_ColorFormatter(log_format, datefmt=datefmt))

root_logger.setLevel(level)
root_logger.addHandler(console_handler)
Comment on lines +61 to +62

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing test test_setup_logging_adds_stream_handler checks that a StreamHandler is added to the logger (the "swvo" package logger), but the new implementation adds handlers to the root logger instead. This test will fail because it's checking the wrong logger's handlers. The test needs to be updated to check logging.getLogger().handlers instead of logger.handlers.

Copilot uses AI. Check for mistakes.

if log_file:
file_handler = logging.FileHandler(log_file, mode=file_mode)

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file_mode parameter is not validated. Invalid values (e.g., "x", "r", or arbitrary strings) will be passed directly to FileHandler(), which could cause runtime errors or unexpected behavior. Consider adding validation to ensure file_mode is one of the valid modes ("w", "a", "x") or documenting the acceptable values in the docstring.

Copilot uses AI. Check for mistakes.

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file handler does not include color formatting, while the console handler uses _ColorFormatter. This is intentional and correct since ANSI color codes should not be written to log files. However, consider adding a comment explaining why different formatters are used for console vs. file handlers to improve code maintainability.

Suggested change
file_handler = logging.FileHandler(log_file, mode=file_mode)
file_handler = logging.FileHandler(log_file, mode=file_mode)
# Use a non-colored formatter for files to avoid writing ANSI escape codes to log files.

Copilot uses AI. Check for mistakes.
file_handler.setFormatter(logging.Formatter(log_format, datefmt=datefmt))
root_logger.addHandler(file_handler)
Comment on lines +64 to +67

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no test coverage for the new file logging functionality. Tests should be added to verify that when log_file is provided, a FileHandler is correctly added to the root logger with the proper formatter and file mode. Additionally, tests should verify that the file_mode parameter works correctly (e.g., testing both "w" and "a" modes).

Copilot uses AI. Check for mistakes.
Loading