Merged
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideWraps database report persistence in a try/except to prevent failures from crashing the CLI run, logging a warning instead when saving fails. Sequence diagram for CLI run with resilient report persistencesequenceDiagram
actor User
participant CLI
participant DBHandler
participant Logger
User->>CLI: run(lat, long, args)
CLI->>CLI: _save_report(ocean_data_dict)
alt db_handler is configured
CLI->>DBHandler: insert_report(ocean_data_dict)
alt insert_report succeeds
DBHandler-->>CLI: success
else insert_report raises Exception
DBHandler-->>CLI: Exception
CLI->>Logger: warning(Failed to save report to database.)
end
else no db_handler
CLI-->>CLI: skip persistence
end
CLI-->>User: render output and exit
Updated class diagram for CLI report persistence and error handlingclassDiagram
class CLI {
+db_handler
+run(lat, long, args)
-_save_report(ocean_data_dict)
-_render_output(ocean_data_dict, arguments)
}
class DBHandler {
+insert_report(ocean_data_dict)
}
class Logger {
+warning(message)
}
CLI --> DBHandler : uses
CLI --> Logger : logs warnings
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Catching a bare
Exceptionmakes it harder to distinguish expected DB issues from programming errors; consider catching a more specific database-related exception type instead. - When logging the failure to save the report, include the exception details (e.g.,
exc_info=Trueor the exception object) so that debugging DB issues is easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Catching a bare `Exception` makes it harder to distinguish expected DB issues from programming errors; consider catching a more specific database-related exception type instead.
- When logging the failure to save the report, include the exception details (e.g., `exc_info=True` or the exception object) so that debugging DB issues is easier.
## Individual Comments
### Comment 1
<location path="src/cli.py" line_range="66-68" />
<code_context>
"""Persists the report to the database if a handler is available."""
if self.db_handler:
- self.db_handler.insert_report(ocean_data_dict)
+ try:
+ self.db_handler.insert_report(ocean_data_dict)
+ except Exception:
+ logger.warning("Failed to save report to database.")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Catching bare `Exception` and only logging a generic warning may hide real errors and lose debugging context.
The try/except around the insert is reasonable, but catching `Exception` and logging only a generic warning can hide real issues. Consider catching the specific DB exception type (e.g. `DatabaseError`), logging the exception details (e.g. with `exc_info=True`), and re-raising unexpected exceptions so they don’t get silently ignored.
Suggested implementation:
```python
def _save_report(self, ocean_data_dict):
"""Persists the report to the database if a handler is available."""
if self.db_handler:
try:
self.db_handler.insert_report(ocean_data_dict)
except DatabaseError:
logger.warning("Failed to save report to database.", exc_info=True)
except Exception:
# Re-raise unexpected exceptions so they are not silently ignored
logger.exception("Unexpected error while saving report to database.")
raise
```
1. Import or define the appropriate `DatabaseError` type used by your DB layer. For example:
- If using Python’s DB-API 2.0 directly: `from sqlite3 import DatabaseError` or from your chosen driver (e.g. `psycopg2`, `mysql.connector`, etc.).
- If using SQLAlchemy: `from sqlalchemy.exc import SQLAlchemyError as DatabaseError`.
2. Ensure `logger` is defined/imported in `src/cli.py` (e.g. `import logging` and `logger = logging.getLogger(__name__)`) if not already present.
3. Optionally, narrow `DatabaseError` further if your DB library exposes more specific exception subclasses (e.g. `IntegrityError`, `OperationalError`) and you want tailored handling.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+66
to
+68
| try: | ||
| self.db_handler.insert_report(ocean_data_dict) | ||
| except Exception: |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Catching bare Exception and only logging a generic warning may hide real errors and lose debugging context.
The try/except around the insert is reasonable, but catching Exception and logging only a generic warning can hide real issues. Consider catching the specific DB exception type (e.g. DatabaseError), logging the exception details (e.g. with exc_info=True), and re-raising unexpected exceptions so they don’t get silently ignored.
Suggested implementation:
def _save_report(self, ocean_data_dict):
"""Persists the report to the database if a handler is available."""
if self.db_handler:
try:
self.db_handler.insert_report(ocean_data_dict)
except DatabaseError:
logger.warning("Failed to save report to database.", exc_info=True)
except Exception:
# Re-raise unexpected exceptions so they are not silently ignored
logger.exception("Unexpected error while saving report to database.")
raise- Import or define the appropriate
DatabaseErrortype used by your DB layer. For example:- If using Python’s DB-API 2.0 directly:
from sqlite3 import DatabaseErroror from your chosen driver (e.g.psycopg2,mysql.connector, etc.). - If using SQLAlchemy:
from sqlalchemy.exc import SQLAlchemyError as DatabaseError.
- If using Python’s DB-API 2.0 directly:
- Ensure
loggeris defined/imported insrc/cli.py(e.g.import loggingandlogger = logging.getLogger(__name__)) if not already present. - Optionally, narrow
DatabaseErrorfurther if your DB library exposes more specific exception subclasses (e.g.IntegrityError,OperationalError) and you want tailored handling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
General:
Code:
Summary by Sourcery
Bug Fixes: