Add custom exception handling for pyrail package #46
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant iRailAPI
participant ExceptionHandler
Client->>iRailAPI: Send Request
iRailAPI-->>ExceptionHandler: Check Response Status
alt Rate Limit Exceeded
ExceptionHandler->>Client: Raise RateLimitError
else Invalid Request
ExceptionHandler->>Client: Raise InvalidRequestError
else Resource Not Found
ExceptionHandler->>Client: Raise NotFoundError
end
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pyrail/exceptions.py (1)
1-19: Consider creating a base exception class and enhancing the exception implementations.The current implementation could be improved by:
- Adding a base exception class
- Enhancing docstrings with more details
- Adding custom initialization for better error messages
Here's a suggested implementation:
"""Module containing custom exception classes for the pyrail package.""" +class PyRailError(Exception): + """Base exception class for all pyrail exceptions.""" + pass -class RateLimitError(Exception): +class RateLimitError(PyRailError): - """Raised when the rate limit is exceeded.""" + """Raised when the API rate limit is exceeded. + + This exception indicates that too many requests have been made to the API + within a short time period. The client should wait before making new requests. + """ - pass + def __init__(self, message="API rate limit exceeded"): + super().__init__(message) -class InvalidRequestError(Exception): +class InvalidRequestError(PyRailError): - """Raised for invalid requests.""" + """Raised when the API request is invalid. + + This exception indicates that the request was malformed or contained + invalid parameters that the API cannot process. + """ - pass + def __init__(self, message="Invalid API request"): + super().__init__(message) -class NotFoundError(Exception): +class NotFoundError(PyRailError): - """Raised when an endpoint is not found.""" + """Raised when the requested resource is not found. + + This exception indicates that the requested station, train, or other + resource does not exist or could not be found in the system. + """ - pass + def __init__(self, message="Resource not found"): + super().__init__(message)pyrail/irail.py (3)
313-314: Remove commented out code.The old retry logic is commented out and should be removed since it's been replaced with the new exception.
raise RateLimitError(f"Rate limit exceeded for method {method}") - #return await self._do_request(method, args)
316-319: Remove commented out code and enhance error message.The old None return is commented out and should be removed. Also, consider extracting the error message from the response JSON if available.
error_message = await response.text() + try: + error_json = await response.json() + error_message = error_json.get('error', error_message) + except ValueError: + pass logger.error("Bad request: %s", error_message) raise InvalidRequestError(error_message) - #return None
13-13: Consider importing the base exception class.If you implement the suggested PyRailError base class, update the imports accordingly.
-from pyrail.exceptions import InvalidRequestError, NotFoundError, RateLimitError +from pyrail.exceptions import ( + PyRailError, + InvalidRequestError, + NotFoundError, + RateLimitError, +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyrail/exceptions.py(1 hunks)pyrail/irail.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run Tests
pyrail/irail.py
[error] 323-323: NotFoundError exception thrown: Stop 'INVALID_STATION' can not be found
[warning] 207-207: PytestDeprecationWarning: The configuration option 'asyncio_default_fixture_loop_scope' is unset
| error_message = await response.text() | ||
| logger.error("Endpoint %s not found, response: %s", method, error_message) | ||
| raise NotFoundError(error_message) | ||
| #return None |
There was a problem hiding this comment.
Fix NotFoundError handling and remove commented code.
The pipeline failure indicates an issue with NotFoundError being thrown for an invalid station. Consider improving the error message to be more specific about the type of resource that wasn't found.
error_message = await response.text()
+ resource_type = "resource"
+ if "station" in method.lower() or "stop" in error_message.lower():
+ resource_type = "station"
+ elif "vehicle" in method.lower():
+ resource_type = "vehicle"
logger.error("Endpoint %s not found, response: %s", method, error_message)
- raise NotFoundError(error_message)
+ raise NotFoundError(f"{resource_type.title()} not found: {error_message}")
- #return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| error_message = await response.text() | |
| logger.error("Endpoint %s not found, response: %s", method, error_message) | |
| raise NotFoundError(error_message) | |
| #return None | |
| error_message = await response.text() | |
| resource_type = "resource" | |
| if "station" in method.lower() or "stop" in error_message.lower(): | |
| resource_type = "station" | |
| elif "vehicle" in method.lower(): | |
| resource_type = "vehicle" | |
| logger.error("Endpoint %s not found, response: %s", method, error_message) | |
| raise NotFoundError(f"{resource_type.title()} not found: {error_message}") |
🧰 Tools
🪛 GitHub Actions: Run Tests
[error] 323-323: NotFoundError exception thrown: Stop 'INVALID_STATION' can not be found
Introduce custom exception classes to improve error handling in the pyrail package, enhancing clarity and control over specific error scenarios.
Summary by CodeRabbit
New Features
Bug Fixes