Skip to content

Conversation

@bybatkhuu
Copy link
Owner

This pull request refactors the configuration handling in the beans_logging package, primarily by replacing the old schemas.py config models with a new, unified config.py implementation. It also cleans up compatibility code for different Pydantic versions, simplifies imports, and introduces a new utility function for slugifying file names. These changes modernize the codebase, improve maintainability, and streamline configuration management.

Configuration Model Refactor:

  • Replaced the entire contents of schemas.py with a new config.py module, consolidating all configuration Pydantic models and validators into a single file. The new models use modern Pydantic v2 features and naming conventions (e.g., LoggerConfigPM, StreamConfigPM). (src/beans_logging/config.py, src/beans_logging/schemas.py) [1] [2]

Import and Naming Consistency:

  • Updated all imports to reference the new config.py models and renamed internal constants modules from _consts to _constants for clarity. (src/beans_logging/__init__.py, src/beans_logging/_base.py, src/beans_logging/_utils.py) [1] [2] [3]

Pydantic Compatibility Cleanup:

  • Removed conditional logic for supporting both Pydantic v1 and v2, standardizing on v2 syntax throughout the codebase (e.g., using only validate_call, model_validator, and field_validator). (src/beans_logging/_base.py, src/beans_logging/_utils.py) [1] [2]

Utility Function Update:

  • Added a new get_slug_name function to _utils.py for generating slugified names from file paths, replacing the previous get_app_name and get_default_logs_dir functions. (src/beans_logging/_utils.py)

Documentation and Type Improvements:

  • Improved type annotations and docstrings for configuration-related methods and constructors, and fixed a small bracket typo in the LoggerLoader constructor. (src/beans_logging/_base.py)

@bybatkhuu bybatkhuu requested a review from Copilot October 2, 2025 08:30
@bybatkhuu bybatkhuu self-assigned this Oct 2, 2025
@bybatkhuu bybatkhuu added the patch [🐛 Fixes] PATCH version label Oct 2, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request modernizes the beans_logging package by consolidating configuration models into a new config.py module and standardizing on Pydantic v2. The refactor replaces the old schemas.py with improved configuration classes, removes Pydantic version compatibility code, and introduces a new utility function for generating slugified names.

  • Replaced schemas.py with a unified config.py containing modernized Pydantic v2 models
  • Removed conditional Pydantic v1/v2 compatibility code throughout the codebase
  • Updated utility functions to use a new get_slug_name function instead of separate get_app_name and get_default_logs_dir functions

Reviewed Changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/beans_logging/schemas.py Completely removed old configuration models (173 lines deleted)
src/beans_logging/config.py Added new unified configuration module with modern Pydantic v2 models
src/beans_logging/_utils.py Updated imports, removed Pydantic compatibility code, replaced utility functions
src/beans_logging/_base.py Updated imports and removed Pydantic version checks
src/beans_logging/init.py Updated imports to reference new config module and renamed constants

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

stream: StreamConfigPM = Field(default_factory=StreamConfigPM)
file: FileConfigPM = Field(default_factory=FileConfigPM)
intercept: InterceptConfigPM = Field(default_factory=InterceptConfigPM)
extra: ExtraConfigPM | None = Field(default_factory=ExtraConfigPM)
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

The extra field has inconsistent typing. It's typed as ExtraConfigPM | None but uses default_factory=ExtraConfigPM which will never produce None. Either change the type to ExtraConfigPM or use default=None if None values are intended.

Suggested change
extra: ExtraConfigPM | None = Field(default_factory=ExtraConfigPM)
extra: ExtraConfigPM = Field(default_factory=ExtraConfigPM)

Copilot uses AI. Check for mistakes.

return (
os.path.splitext(os.path.basename(sys.argv[0]))[0]
if not file_path:
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

The condition if not file_path: will be True for empty strings, which may not be the intended behavior. Consider using if file_path is None: to only handle the case where no path is explicitly provided.

Suggested change
if not file_path:
if file_path is None:

Copilot uses AI. Check for mistakes.
@bybatkhuu bybatkhuu merged commit bc2645b into main Oct 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

patch [🐛 Fixes] PATCH version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants