Skip to content

Minor improvements to spotify plugin typing. #5815

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

semohr
Copy link
Contributor

@semohr semohr commented Jun 2, 2025

Description

Added some more typehints to the spotify plugin. Also added a method to get the tokenfile and changed to logic for the handle_response to use requests.request.

This is done mainly to prepare for #5787, see also #5814

@Copilot Copilot AI review requested due to automatic review settings June 2, 2025 12:09
Copy link

github-actions bot commented Jun 2, 2025

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.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds type hints, refactors token file handling, and updates the internal request logic to use requests.request with retry handling.

  • Introduces a _tokenfile() helper for resolving the OAuth token path
  • Adds literal and return-type annotations across the plugin
  • Refactors _handle_response to use a generic request method and updates retry logic
Comments suppressed due to low confidence (1)

beetsplug/spotify.py:200

  • Unhandled HTTP status codes other than 401, 404, and 429 will be swallowed and cause _handle_response to return None. Add a fallback branch (e.g. an else after the elif chain) to raise a SpotifyAPIError for any unexpected status codes to avoid silent failures.
except requests.exceptions.RequestException as e:

Copy link
Member

@wisp3rwind wisp3rwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small nitpicks, otherwise this looks good to me 👍 I particularly like the request_type refactor, there's really no need to pass around functions here.

I'm not up to date on the related in-flight PRs on refactoring metadata source plugins in general, so I only looked at this in isolation. I suppose mypy's complaints about the _search_api signature would be resolved via those PRs?

@semohr
Copy link
Contributor Author

semohr commented Jun 4, 2025

I'm not up to date on the related in-flight PRs on refactoring metadata source plugins in general, so I only looked at this in isolation. I suppose mypy's complaints about the _search_api signature would be resolved via those PRs?

The typing issue should be fixed once the abstract base class used by the spotify plugin changes. I might need to revisit this tho. I felt like the requests.request addition does not fit the metadataplugin PR thus I opened this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants