Refactor: Move logging.getLogger to class static #65
Conversation
WalkthroughThe pull request modifies the logging implementation in the Changes
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
It doesn't need to be instance specific, let it be class specific
glensc
force-pushed
the
logging-class-static
branch
from
41f9f87 to
1dc2f73
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
trakt/api.py(2 hunks)
🔇 Additional comments (1)
trakt/api.py (1)
179-179: LGTM! Consistent logger implementation across classes.The change maintains consistency with HttpClient class and follows the same best practices.
Let's verify all logger references are updated:
✅ Verification successful
✓ Logger implementation verified and correct
All logger references in TokenAuth class are properly using the class-level logger, maintaining consistency with HttpClient class implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instance-level logger references in TokenAuth # Expect: Only class-level logger references # Search for any remaining self.logger references in TokenAuth class ast-grep --pattern $'class TokenAuth { $$$ self.logger $$$ }' # Search for class-level logger usage in TokenAuth ast-grep --pattern $'class TokenAuth { $$$ logger = logging.getLogger $$$ }'Length of output: 152
Script:
#!/bin/bash # First verify the file exists and show its content echo "=== File content ===" cat trakt/api.py echo -e "\n=== Logger usage ===" # Search for any logger-related lines with context rg "logger" trakt/api.py -B 2 -A 2 echo -e "\n=== TokenAuth class definition ===" # Find the TokenAuth class with some context rg "class TokenAuth" trakt/api.py -B 2 -A 5Length of output: 11924
| @@ -19,6 +19,8 @@ class HttpClient: | |||
| """Class for abstracting HTTP requests | |||
| """ | |||
|
|
|||
| logger = logging.getLogger(__name__) | |||
There was a problem hiding this comment.
💡 Codebase verification
Warning: Incomplete logger refactoring detected
The logger was moved to class level, but all references still use self.logger. This will cause runtime errors as the instance-level logger no longer exists. Update these references:
self.logger.debug('REQUEST [%s] (%s)', method, url)self.logger.debug('RESPONSE [%s] (%s): %s', method, url, str(response))self.logger.debug("Skipping auth headers: missing credentials")self.logger.error("Max token refresh attempts reached. Manual intervention required.")self.logger.info("OAuth token has expired, refreshing now...")- Two more
self.loggercalls (debug and info)
Replace all self.logger with logger to use the class-level logger.
🔗 Analysis chain
LGTM! Moving logger to class level is a good practice.
Moving the logger to class level is more efficient as it's shared across instances and follows Python logging best practices by using the module's namespace.
Let's verify all logger references are updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instance-level logger references
# Expect: Only class-level logger references
# Search for any remaining self.logger references
rg 'self\.logger' trakt/api.py
# Search for class-level logger usage
rg 'logger\s*=' trakt/api.py
Length of output: 588
There was a problem hiding this comment.
@coderabbitai: You are wrong. class instance must be still accessed via self.logger
There was a problem hiding this comment.
@glensc The test results confirm your point. Both access patterns work, and I appreciate the correction. This helps maintain better code review quality going forward.
🔗 Analysis chain
@glensc I apologize for my mistake. You are absolutely correct. Class attributes in Python can be accessed via self, and my suggestion to replace self.logger was incorrect.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a test script to verify class attribute access behavior
cat << 'EOF' > test.py
class Test:
x = 1
def print_x(self):
# Access via self
print(f"via self: {self.x}")
# Access directly
print(f"direct: {Test.x}")
t = Test()
t.print_x()
EOF
python3 test.py
Length of output: 45
✏️ Learnings added
Learnt from: glensc
PR: glensc/python-pytrakt#65
File: trakt/api.py:22-22
Timestamp: 2025-01-31T07:23:53.654Z
Learning: In Python, class-level attributes (like `logger = logging.getLogger(__name__)`) can and should be accessed via `self` within instance methods. Using `self.logger` is the correct way to access such class attributes, even though they are defined at class level.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
It doesn't need to be instance-specific, let it be class-specific.