-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Issue#5998 #6004
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
Issue#5998 #6004
Conversation
…rors-in-beets Fix lyrics plugin JSONDecodeError handling
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIntroduce a compatibility fallback for JSONDecodeError when using older versions of requests and update the lyrics plugin to use this alias in its exception handling. Class diagram for updated exception handling in lyrics pluginclassDiagram
class LyricsPlugin {
+post_json(url: str, params: JSONDict | None = None, **kwargs)
+handle_request() Iterator[None]
}
class NotFoundError {
<<inherits>> requests.exceptions.HTTPError
}
class JSONDecodeError
LyricsPlugin --|> NotFoundError
LyricsPlugin ..> JSONDecodeError : uses
Flow diagram for JSONDecodeError fallback logicflowchart TD
A["Check if requests.exceptions has JSONDecodeError"] -->|Yes| B["Use requests.exceptions.JSONDecodeError"]
A -->|No| C["Use json.JSONDecodeError from stdlib"]
B & C --> D["Assign to shared alias JSONDecodeError"]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Move the JSONDecodeError alias closer to the top of the module (with the other imports) to improve readability.
- Consider using a try/except import block for JSONDecodeError rather than getattr for a more conventional fallback pattern.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Move the JSONDecodeError alias closer to the top of the module (with the other imports) to improve readability.
- Consider using a try/except import block for JSONDecodeError rather than getattr for a more conventional fallback pattern.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
snejus
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.
Thanks for addressing this! I think, we'd rather keep using requests.JSONDecodeError unconditionally in lyrics.py and instead update the requests dependency requirement to ensure that users do not install outdated versions.
Would you be happy to do so?
- Update
requestsversion requirement to require the version thatJSONDecodeErrorwas introduced as the minimum (I think it was>=2.27.0). - Run
uv tool run poetry<2 update requests
Summary
Testing
pre-commit run --files beetsplug/lyrics.py`