REFACTOR: Replace prints with proper logging#1036
REFACTOR: Replace prints with proper logging#1036Diegomed11 wants to merge 4 commits intoalphaonelabs:mainfrom
Conversation
👀 Peer Review RequiredHi @Diegomed11! This pull request does not yet have a peer review. Before this PR can be merged, please request a review from one of your peers:
Thank you for contributing! 🎉 |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Note
|
| Cohort / File(s) | Summary |
|---|---|
Models (geolocation) web/models.py |
Added import logging and logger = logging.getLogger(__name__); replaced print(...) with logger.debug(...) for coordinate saves and used logger.exception(...) in the exception path of Session.fetch_coordinates. |
Quiz views web/quiz_views.py |
Added module-level logger and replaced print(e) in add_question error handling with logger.error("An error occurred: %s", e). |
General views / handlers web/views.py |
Added module-level logger and replaced various print calls with logger.warning, logger.debug, and logger.exception for Slack webhook checks, form validation, SendGrid/Twitter/connectivity errors, and contributor fetch failures. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately captures the main refactoring objective: replacing print statements with logging across multiple files in the codebase. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
Tip
CodeRabbit can suggest fixes for GitHub Check annotations.
Configure the reviews.tools.github-checks setting to adjust the time to wait for GitHub Checks to complete.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/models.py`:
- Around line 498-499: Replace the manual error logging in the except block that
calls logger.error("Error geocoding session %s location '%s': %s", self.id,
self.location, str(e)) with logger.exception so the full stack trace is
recorded; locate the exception handler in web/models.py that references self.id,
self.location and the exception variable e (the geocoding error path) and change
the logging call to use logger.exception with an appropriate message (omit
str(e) since logger.exception captures the exception and traceback
automatically).
- Around line 494-497: The logger.debug call incorrectly passes two f-strings as
separate positional arguments so the second is ignored; replace it with a single
lazy-formatted logging call such as logger.debug("Skipping session %s due to
invalid coordinates: lat=%s, lng=%s", self.id, self.latitude, self.longitude) to
use the logging module's %s placeholders and keep consistent lazy formatting for
the logger.debug invocation that references self.id, self.latitude and
self.longitude.
In `@web/quiz_views.py`:
- Around line 187-190: Replace the logger.error call inside the except block
that currently logs "An error occurred: %s" with logger.exception so the stack
trace is captured automatically; locate the except handler in web/quiz_views.py
where logger.error("An error occurred: %s", e) is used, change it to
logger.exception("An error occurred") (do not pass the exception object), and
keep the subsequent raise to re-raise the exception.
In `@web/views.py`:
- Line 8116: Replace uses of logger.error("Error posting tweet: %s", e) inside
the exception handlers with logger.exception(...) so the traceback is captured
automatically; locate the catch blocks that call logger.error with the exception
variable (e.g., the "Error posting tweet" logger.error call and the similar
logger.error call near the other reported location) and change them to
logger.exception("Error posting tweet") (or an appropriate message) without
manually formatting the exception variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 175e5be7-b911-4d2a-a346-b394dc02021f
📒 Files selected for processing (3)
web/models.pyweb/quiz_views.pyweb/views.py
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
web/models.py (1)
494-497:⚠️ Potential issue | 🟠 MajorBug: The second f-string argument is being silently discarded.
This issue was flagged in a previous review but appears to still be present. When you pass multiple positional arguments to
logger.debug(), the first argument is treated as a format string with%splaceholders, and subsequent arguments fill those placeholders. Since your first f-string has no placeholders, the second f-string (f"lat={self.latitude}, \n lng={self.longitude}") is never included in the log output.Additionally, the static analysis tool is flagging
G004(logging statement uses f-string), which this fix would also resolve.,
🐛 Suggested fix using lazy formatting
else: - logger.debug( - f"Skipping session {self.id} due to invalid coordinates:", - f"lat={self.latitude}, \n lng={self.longitude}", - ) + logger.debug( + "Skipping session %s due to invalid coordinates: lat=%s, lng=%s", + self.id, self.latitude, self.longitude, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/models.py` around lines 494 - 497, The logger.debug call currently passes two f-strings so the second is discarded and triggers G004; change the call in the class where logger.debug is invoked (referencing self.id, self.latitude, self.longitude) to use a single format string with lazy logging placeholders and pass the id/latitude/longitude as separate arguments (e.g. one format string describing the session id and coordinates and then the three values), ensuring the full message is logged and avoiding f-strings in the logging call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/models.py`:
- Around line 494-497: The logger.debug call currently passes two f-strings so
the second is discarded and triggers G004; change the call in the class where
logger.debug is invoked (referencing self.id, self.latitude, self.longitude) to
use a single format string with lazy logging placeholders and pass the
id/latitude/longitude as separate arguments (e.g. one format string describing
the session id and coordinates and then the three values), ensuring the full
message is logged and avoiding f-strings in the logging call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 637f6806-2376-47b9-a781-39e368435ee8
📒 Files selected for processing (1)
web/models.py
|
Fixed in the latest commit: switched to logger.exception() in all except blocks for automatic traceback capture. |
github-actions
bot
dismissed
their stale review
All review conversations have been resolved.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/views.py`:
- Line 3257: The warning-level log of validation failures should be lowered to
debug to avoid noisy logs; change the logger.warning("Form errors: %s",
form.errors) call in this view to logger.debug("Form errors: %s", form.errors)
(or gate it behind a debug/config flag) so validation errors (from the form
variable) are recorded at debug level rather than warning.
- Line 1: The stdlib import "import logging" is out of isort order; update the
import block in web/views.py so standard library imports are sorted per isort
Black profile (or run isort --profile=black) ensuring "import logging" is placed
in the correct stdlib group and reorder surrounding imports accordingly; re-run
linters to confirm the import order passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ea30885-3b68-4f09-9980-fffd9f27eab8
📒 Files selected for processing (1)
web/views.py
|
@A1L13N ready for review. Completes logging cleanup across models.py, quiz_views.py, and views.py. All feedback addressed. |
1 participant
Description
Replaced the remaining
print()statements across the codebase (web/views.py,web/models.py, andweb/quiz_views.py) with Python'sloggingmodule to follow production best practices. Used lazy formatting (%s) and appropriate log levels (logger.debug,logger.warning,logger.error) based on the context of each message. Also resolvedflake8linting errors regarding import order and spacing to ensure clean code.Related issues
No existing issue — identified proactive technical debt/improvement during codebase exploration.
Checklist
Purpose
Refactor: replace direct print() statements with Python's logging module across the codebase to improve debugging, consistency, and adherence to best practices.
Key Changes
web/models.py
logger = logging.getLogger(__name__)) and importedlogging.print()calls inSession.fetch_coordinates()withlogger.debug()for normal/invalid-coordinate paths.logger.exception()to log errors with tracebacks.web/quiz_views.py
print(e)in exception handling withlogger.error("An error occurred: %s", e)using lazy formatting.web/views.py
import logging.print()statements with appropriate logging calls (logger.warning(),logger.debug(),logger.error(), andlogger.exception()).Impact
logger.exception()in except blocks to ensure stack traces are captured.Size / Review Notes