-
Notifications
You must be signed in to change notification settings - Fork 20
Implement custom exceptions #60
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
Conversation
aditeyabaral
left a comment
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.
We should catch parsing errors in get_profile_informaiton. If the website changes layout, then this part will silently fail without information.
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.
Pull Request Overview
This PR implements custom exceptions for handling all errors in PESUAcademy-related functionality, replacing generic error handling with structured exception types derived from a base PESUAcademyError class.
- Introduces authentication-specific exceptions (
AuthenticationError,CSRFTokenError,ProfileFetchError,ProfileParseError) - Refactors error handling in
app/pesu.pyto use structured exceptions instead of genericExceptiontypes - Updates tests to expect the new exception types and adjusts response format assertions
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/exceptions/base.py | Defines the base PESUAcademyError exception class with message and status code |
| app/exceptions/authentication.py | Implements specific authentication-related exception classes |
| app/pesu.py | Refactors error handling to use custom exceptions instead of generic ones |
| app/app.py | Adds exception handler for PESUAcademyError and updates error response format |
| tests/ | Updates test assertions to expect new exception types and response formats |
Comments suppressed due to low confidence (4)
app/exceptions/base.py:1
- The class name 'PESUAcademyError' is inconsistent with the PR description which mentions 'PESUAuthError' as the base class.
class PESUAcademyError(Exception):
tests/unit/test_pesu.py:18
- The test expects ProfileFetchError to be raised but still tries to assert on the result. Since an exception is expected, the result assignment and assertions on lines 19-20 are unreachable and should be removed.
result = pesu.get_profile_information(MagicMock(), "testuser")
tests/unit/test_pesu.py:29
- Similar to the previous test, this code tries to access result after expecting an exception. Lines 30-31 are unreachable and should be removed.
result = pesu.get_profile_information(MagicMock(), "testuser")
tests/unit/test_pesu.py:40
- This test expects CSRFTokenError but still asserts on result. Lines 41-42 are unreachable after the exception is raised and should be removed.
result = pesu.authenticate("testuser", "testpass")
Use custom exceptions to handle all errors thrown by PESUAcademy-related functionality in
app/pesu.py. The custom exceptions are derived from the basePESUAuthErrorclass inapp/exceptions/base.py. For now all usable exceptions are inapp/exceptions/exceptions.pywith the option of splitting each into its own file later on if required.