Use the created logger and not the root logger #34
Conversation
WalkthroughThe recent changes include updating the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![senior-dev-bot[bot]](https://avatars.githubusercontent.com/in/282840?s=60&v=4){kind=small}
| *.json | ||
| cookies.txt | ||
| .coverage | ||
| .envrc |
There was a problem hiding this comment.
CODE REVIEW
It appears you've added .envrc which is useful for environment variable management but this type of file should not be part of the version control to avoid accidentally exposing sensitive information. Consider adding .envrc to your .gitignore to prevent it from being tracked. Here's how you can do it:
echo '.envrc' >> .gitignore
| action = login_form.get("_action") | ||
| login_form.pop("_action") | ||
| else: | ||
| logging.debug( | ||
| logger.debug( | ||
| f"url: {res.url}, status: {res.status_code}\nhtml:\n{res.text}" | ||
| ) | ||
| raise ValueError("Failed to find login form") |
There was a problem hiding this comment.
CODE REVIEW
The change from logging.debug to logger.debug suggests you've switched from using the module-level logging to an instance of a logger. It's crucial to ensure logger is properly configured to handle messages at the debug level. Also, consider sanitizing sensitive information from res.text before logging, for security reasons.
Example:
# Ensure `logger` is set up correctly
logger.setLevel(logging.DEBUG)
# When logging responses, avoid logging sensitive information
logger.debug(f"url: {res.url}, status: {res.status_code}")| ) | ||
|
|
||
| if not self.is_logged_in: | ||
| logging.debug( | ||
| logger.debug( | ||
| f"url: {res.url}, status: {res.status_code}\nhtml:\n{res.text}" | ||
| ) | ||
| raise ValueError("Login failed") |
There was a problem hiding this comment.
CODE REVIEW
Good job updating from logging to a more instance-specific logger object which likely offers better control and configuration suited to this context. However, ensure logger is properly defined and configured in this class or module. It might also be prudent to censor or limit the amount of sensitive information (like full HTML content) logged, as it can potentially include sensitive data, impacting both performance and security.
if not self.is_logged_in:
logger.debug(f"url: {res.url}, status: {res.status_code}")
raise ValueError("Login failed")| with open(cookiefile, "r") as f: | ||
| return json.load(f) | ||
| except (FileNotFoundError, json.decoder.JSONDecodeError) as e: | ||
| logging.debug(f"Error reading cookies from {cookiefile}: {e}") | ||
| logger.debug(f"Error reading cookies from {cookiefile}: {e}") | ||
| return {} | ||
|
|
||
|
|
There was a problem hiding this comment.
CODE REVIEW
The update from logging to logger suggests a move to a more modular or instance-specific logging, which is a good practice for manageability and granularity in larger applications. Just ensure that the logger object is properly configured elsewhere in your code to handle debug-level messages. Additionally, consider enhancing error handling by not just logging but also informing the calling function or user in a way that does not disrupt the user experience or system stability. For example:
try:
with open(cookiefile, "r") as f:
return json.load(f)
except (FileNotFoundError, json.decoder.JSONDecodeError) as e:
logger.debug(f"Error reading cookies from {cookiefile}: {e}")
# Potentially consider a more user-friendly error strategy here
return {}| if login_link: | ||
| login_url = urljoin(res.url, login_link.get("href")) | ||
| else: | ||
| logging.debug( | ||
| logger.debug( | ||
| f"url: {res.url}, status: {res.status_code}\nhtml:\n{res.text}" | ||
| ) | ||
| raise ValueError("Failed to find login url") |
There was a problem hiding this comment.
CODE REVIEW
The change from logging.debug to logger.debug suggests a move from the module-level logging to an instance (or named) logger, which is generally good for more granular control over logging. However, ensure that logger is properly set up earlier in the code. Also, consider whether including the entire response text (res.text) in the logging is always safe and necessary, as it can be too verbose or include sensitive information. If the response text is essential for debugging, ensure it's handled securely.
if login_link:
login_url = urljoin(res.url, login_link.get("href"))
else:
logger.debug(f"url: {res.url}, status: {res.status_code}") # Consider logging `res.text` conditionally or securely.
raise ValueError("Failed to find login url")
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .gitignore (1 hunks)
- moocfi_cses.py (4 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Additional comments not posted (3)
moocfi_cses.py (3)
56-58: Use oflogger.debuginstead oflogging.debugis correct and ensures the use of the custom logger.
67-69: Use oflogger.debuginstead oflogging.debugis correct and ensures the use of the custom logger.
197-197: Use oflogger.debuginstead oflogging.debugis correct and ensures the use of the custom logger.
Summary by CodeRabbit
Chores
.gitignoreto ignore.envrcfiles.Refactor