-
Notifications
You must be signed in to change notification settings - Fork 1.9k
spotify: Handle API failures #5910
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
base: master
Are you sure you want to change the base?
Conversation
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
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 improves error handling in the Spotify plugin by adding proper exception handling around API calls that previously could crash during imports. The changes wrap critical Spotify API interactions with try-catch blocks to gracefully handle API failures.
Key changes:
- Added exception handling for API failures in album and track retrieval methods
- Modified
track_info
method to returnNone
on API errors instead of crashing - Updated type annotations to reflect the new nullable return types
def track_info(self, track_id: str): | ||
def track_info( | ||
self, track_id: str | ||
) -> Tuple[Any | None, Any | None, Any | None, Any | None] | None: |
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.
[nitpick] The return type annotation is verbose and unclear. Consider creating a type alias or using a more descriptive approach, such as TrackInfoTuple | None
where TrackInfoTuple = Tuple[Optional[Any], Optional[Any], Optional[Any], Optional[Any]]
.
) -> Tuple[Any | None, Any | None, Any | None, Any | None] | None: | |
) -> TrackInfoTuple | None: |
Copilot uses AI. Check for mistakes.
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
dc5535d
to
08b239f
Compare
try: | ||
album_data = self._handle_response( | ||
"get", self.album_url + spotify_id | ||
) | ||
except APIError: | ||
return None | ||
|
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.
This approach introduces a lot of duplication in the code. I think a cleaner approach would be to never raise APIError in the first place.
See _handle_response
method - I'd suggest removing all lines that raise APIError
and returning None
instead.
Maybe it is worth taking a step back here: How do we want to handle errors in metadata plugins in general? E.g. what should happen if one plugin fails because a remote service is currently unreachable? Do we want beets to crash here or just warn a user? I think raising errors in the specific metadata plugin (such as spotify or deezer) is completely fine. We should handle errors globally for any metadata plugins tho and warn users. Proposal: Let's add error handling to the |
Lots of these weren't handled, leading to crashes while importing.
08b239f
to
55b3e45
Compare
Lots of these weren't handled, leading to crashes while importing.
To Do
docs/
to describe it.)docs/changelog.rst
to the bottom of one of the lists near the top of the document.)