-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
feat: add exceptions #15
Conversation
Signed-off-by: Prati28 <[email protected]>
Reviewer's Guide by SourceryThis pull request introduces custom exception handling to the Tus Storage Handler. A new file File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @psankhe28 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding unit tests to verify the behavior of these new exceptions.
- It would be helpful to include docstrings or comments explaining when and how these exceptions are used in the codebase.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 8 8
=========================================
Hits 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Prati28 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @psankhe28 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding unit tests and documentation for the new exception handling to ensure proper functionality and usage.
- The GenericException and catch-all Exception seem redundant. Consider using more specific exception types instead.
- Please provide more context about issue feat: exceptions in case of errors #14 in the PR description to clarify the problem this change is addressing.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
from werkzeug.exceptions import BadRequest, InternalServerError, NotFound | ||
|
||
|
||
class GenericException(Exception): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider removing or enhancing GenericException
The GenericException class doesn't add any functionality beyond the built-in Exception. Consider removing it or adding specific behavior if it's needed.
class TusStorageHandlerException(Exception):
"""Base exception for the Tus Storage Handler."""
def __init__(self, message="An error occurred in the Tus Storage Handler"):
self.message = message
super().__init__(self.message)
pass | ||
|
||
|
||
exceptions = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Revise error messages for clarity and professionalism
Some error messages, like the one for BadRequest, could be more professional and informative. Consider revising these to provide more specific and actionable information.
exceptions = {
Exception: {
"message": "An unexpected error occurred. Please try again or contact support.",
},
BadRequest: {
"message": "Invalid request. Please check your input and try again.",
},
NotFound: {
"message": "The requested resource could not be found.",
},
Unauthorized: {
"message": "Authentication required. Please log in and try again.",
},
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include exceptions in the FOCA setup PR (#13) and close this PR
Description
Fixes #14
Checklist
of this project, including, in particular, with regard to any style guidelines
Conventional Commits specification; in particular, it clearly
indicates that a change is a breaking change
using the PR title as the commit message
changed behavior
or updated existing ones (only for Python, TypeScript, etc.)
(Google-style Python docstrings) for all
packages/modules/functions/classes/methods or updated existing ones
works
Comments
Summary by Sourcery
Add a new module for handling exceptions in the Tus Storage Handler, including custom messages and status codes for various error types.
New Features: