-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Improve ListenBrainz error handling and simplify playlist handling #5480
Conversation
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.
I commented under the change that is required to fix #5459.
Could you by any chance submit the rest of the changes in a separate PR as they seem to be a more general refactor?
Also, I suspect you are using black to do auto formatting - note that the max line length is configured as 80 across the project.
This should have been caught by CI, but now I see that ruff
only shouts for line lengths longer than 88.
Co-authored-by: Šarūnas Nejus <snejus@protonmail.com>
@snejus Made the changes. It looks like the check is failing for unrelated reasons. |
Thank you! The fix looks fine to me.
Above I asked to only keep the fix to the issue in this PR. That's because this plugin is not tested, so there's no way to verify that a refactor did not introduce any unwanted effects. If you do want to keep the rest of the changes here, would you be happy adding some tests? |
Tests are not my strong suit unfortunately... The current version is broken anyways, so I don't think we can be worse off. I'm here in case there are any issues that come up. I use this plugin regularly, so trust me I'll be the first to identify an issue 😜 |
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 is a nice and simple little change, thanks @arsaboo!
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.
Okay, let's go then!
Description
Fixes #5459
To Do
docs/changelog.rst
to the bottom of one of the lists near the top of the document.)