Skip to content
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

Jump to bottom

Use the created logger and not the root logger #34

Merged
merged 1 commit into from
May 14, 2024
Merged

Use the created logger and not the root logger #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ __pycache__
*.json
cookies.txt
.coverage
.envrc
Comment on lines 6 to +9
Copy link

Choose a reason for hiding this comment

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

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

8 changes: 4 additions & 4 deletions moocfi_cses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def login(self) -> None:
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")
Comment on lines 53 to 59
Copy link

Choose a reason for hiding this comment

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

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")

Expand All @@ -64,7 +64,7 @@ def login(self) -> None:
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")
Comment on lines 64 to 70
Copy link

Choose a reason for hiding this comment

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

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}")

Expand All @@ -79,7 +79,7 @@ def login(self) -> None:
)

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")
Comment on lines 79 to 85
Copy link

Choose a reason for hiding this comment

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

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")

Expand Down Expand Up @@ -194,7 +194,7 @@ def read_cookie_file(cookiefile: str) -> dict[str, str]:
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 {}


Comment on lines 194 to 200
Copy link

Choose a reason for hiding this comment

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

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 {}

Expand Down