-
Notifications
You must be signed in to change notification settings - Fork 2
Fix error introduced in 1.44.29 release #3
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.
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
base: master
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUncomments the logging setup path so that a logger is always configured, and removes a stray self-call in the test helper that could cause recursive execution or side effects when importing or running tests. Sequence diagram for updated setup_logging_custom behaviorsequenceDiagram
actor Developer
participant App
participant LoggerModule
participant logging
Developer->>App: import richcolorlog
App->>LoggerModule: setup_logging_custom(name, level, other_params)
LoggerModule->>LoggerModule: check environment
alt NO_LOGGING not set
LoggerModule->>logging: basicConfig(level)
else NO_LOGGING set
LoggerModule->>logging: basicConfig(level=CRITICAL)
end
LoggerModule->>logging: getLogger(name)
logging-->>LoggerModule: logger instance
LoggerModule->>LoggerModule: assign CustomLogger class
LoggerModule-->>App: configured CustomLogger instance
Flow diagram for updated setup_logging_custom and test helperflowchart TD
A["Call setup_logging_custom"] --> B["Set environment NO_LOGGING based on parameters"]
B --> C{"NO_LOGGING is set?"}
C -->|Yes| D["logging.basicConfig(level=CRITICAL)"]
C -->|No| E["logging.basicConfig(level from parameters)"]
D --> F["logger = logging.getLogger(name)"]
E --> F
F --> G["Set logger.__class__ = CustomLogger"]
G --> H["Return configured logger"]
subgraph Test_helper_changes
T1["Define test() helper function"] --> T2["Previously: implicit call to test() at end of file"]
T2 --> T3["Potential recursive execution or side effects"]
T1 --> T4["Now: no automatic call to test()"]
T4 --> T5["test() runs only when explicitly invoked"]
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- Instead of commenting out the
_check_logging_disabled()block, either remove it entirely or replace it with a clearer, configurable condition (e.g., a function parameter or explicit flag) so the logging-disable behavior is well defined rather than left as dead code. - By removing the early return when logging is disabled,
setup_logging_customnow always mutates the logger intoCustomLogger; if this change is intentional, consider making that behavior explicit in the function name or signature so callers understand it no longer respects the previous disable mechanism.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of commenting out the `_check_logging_disabled()` block, either remove it entirely or replace it with a clearer, configurable condition (e.g., a function parameter or explicit flag) so the logging-disable behavior is well defined rather than left as dead code.
- By removing the early return when logging is disabled, `setup_logging_custom` now always mutates the logger into `CustomLogger`; if this change is intentional, consider making that behavior explicit in the function name or signature so callers understand it no longer respects the previous disable mechanism.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai shut up. |
Since I see You don't read Issues, hopefully you will read this PR. Reported issue many weeks ago
#2
Called function was commented out in 1.44.29, but this call still existed causing error in some instances.