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

resolve transl-tracklisting relations for pseudo releases #4714

Merged
merged 5 commits into from
Jun 26, 2023

Conversation

TypicalFence
Copy link
Contributor

Description

Fixes #654

As in the issue suggested I implemented the "easiest/most obvious approach".

It fetches release relations and "follows" the ones for the type transl-tracklisting for pseudo releases. The additional data gets merged with the pseudo-release. I am not 100% sure if I included all applicable fields for the merge. I left the pseudo-releases id's as is.

This change makes it near impossible to differentiate the pseudo releases from the actual releases in the ui, so I added that to disambig_string. Are there any unwanted side effects from that change?

A small screenshot for reference:
Screenshot 2023-03-15 at 22 42 24

This implementation has almost the same approach as the plugin I wrote recently. Said plugin hasn't caused any issues for me yet. This PR handles things much cleaner tho (it doesn't have to fetch pseudo-releases twice for instance).

I feel like my code ended up being a bit awkward, I haven't written any serious python in a long while and pep8 feels a bit tedious.

To Do

  • Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
  • Tests. (Encouraged but not strictly required.)
  • Add a Setting for people who really don't care about this?

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Super cool; thank you for getting this started!!! Here are just a few code-level suggestions.

beets/autotag/mb.py Outdated Show resolved Hide resolved
beets/autotag/mb.py Outdated Show resolved Hide resolved
beets/autotag/mb.py Outdated Show resolved Hide resolved
beets/autotag/mb.py Show resolved Hide resolved
beets/ui/commands.py Outdated Show resolved Hide resolved
@sampsyo
Copy link
Member

sampsyo commented Apr 7, 2023

Awesome! It looks like there is one minor conflict with a recent change to RELEASE_INCLUDES—any chance you could take a shot at resolving that? Then perhaps we'll have a clean merge.

@TypicalFence
Copy link
Contributor Author

@sampsyo I'll try to get around rebasing soon

@TypicalFence
Copy link
Contributor Author

hey @sampsyo sorry that it took months but I've rebase the branch

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Awesome; this is looking great!! Could you please just add a really brief changelog entry? There is also a super minor lint thing (a long line that will be easy to wrap).

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Many thanks again for tracking this down!

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.

Follow MusicBrainz pseudo-releases
2 participants