Skip to content
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

Merged
merged 7 commits into from
Nov 13, 2024

Conversation

arsaboo
Copy link
Contributor

@arsaboo arsaboo commented Oct 25, 2024

Description

Fixes #5459

To Do

  • Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
  • Tests. (Very much encouraged but not strictly required.)

Copy link
Member

@snejus snejus left a 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.

beetsplug/listenbrainz.py Outdated Show resolved Hide resolved
@arsaboo
Copy link
Contributor Author

arsaboo commented Nov 11, 2024

@snejus Made the changes. It looks like the check is failing for unrelated reasons.

@snejus
Copy link
Member

snejus commented Nov 11, 2024

Thank you! The fix looks fine to me.

Could you by any chance submit the rest of the changes in a separate PR as they seem to be a more general refactor?

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?

@arsaboo
Copy link
Contributor Author

arsaboo commented Nov 11, 2024

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 😜

Copy link
Member

@bal-e bal-e left a 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!

beetsplug/listenbrainz.py Show resolved Hide resolved
Copy link
Member

@snejus snejus left a 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!

@snejus snejus merged commit 7e9f7fc into beetbox:master Nov 13, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Listenbrainz broken - AttributeError
3 participants