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.

Sign up for GitHub

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

Jump to bottom

Refactor metadata source ID extraction utilities #4633

Merged
merged 9 commits into from
Mar 8, 2023
Merged

Refactor metadata source ID extraction utilities #4633

merged 9 commits into from
Mar 8, 2023

Conversation

JOJ0
Copy link
Member

@JOJ0 JOJ0 commented Jan 11, 2023 •
edited
Loading

Description

Tidy up and streamline ID extraction utilities used by metadata source plugins. Enable the usage of those utilities from everywhere.

  • Introduce a new module beets.util.id_extractors
  • Move the Discogs ID extractor function to the new module.
  • Move the regex variables for the following services to the new modul:
    • Spotify
    • Deezer
    • Beatport
    • Leave a note about Bandcamp ID's
  • Refactor the MetadataSourcePlugin._get_id() to use the new utility location.
  • Fix the Beatport plugin to use the shared functionality of MetadataSourceplugin which it already inherits.
  • Fix the Discogs ID extractor function to support a legacy format that is still in use.

To Do

  • Documentation.
  • Changelog. Didn't add one for now. Does it make sense for such an "internal change"? We don't change anything from a user perspective.
  • Tests.

@JOJ0 JOJ0 mentioned this pull request Jan 11, 2023
Closed
3 tasks
@JOJ0
Copy link
Member Author

JOJ0 commented Jan 11, 2023 •
edited
Loading

One idea for future use of this "refactoring PR" is #4474, which currently is set back to draft state until I get to rebasing to only include the relevant parts.

@JOJ0
Copy link
Member Author

JOJ0 commented Jan 11, 2023 •
edited
Loading

@sampsyo, this one is for you <3, hope you like it. Suggestions on "naming things" and documentation/changelog are very welcome as well as any other thoughts.

@JOJ0 JOJ0 force-pushed the refactor_id_extraction branch 2 times, most recently from d0e96d8 to 5d85ab1 Compare January 11, 2023 12:13
@JOJ0
Copy link
Member Author

JOJ0 commented Feb 1, 2023

Hi @sampsyo, I know this one is probably a low priority one since it actually doesn't change any behaviour, nor does it fix anything (except a little oversight in the currently defunct beatport plugin). Certainly I ultimately did this to get a better view of what #4474 will actually change.
I think merging this one is worthwile in any case since it just does some tidying up and most of all I think "it's a rather low hanging fruit" :-)

  • Tests are fixed.
  • One remaining question: Do you think a changelog would be good? Something like "internal change/reorganization of metadata ID extraction" - something like that - or not too important and just leave it out?

JOJ0 added 6 commits March 8, 2023 18:12
@JOJ0
Introduce new module beets.util.id_extractors
0175a9a 
- We introduce a new submodule of beets.util named id_extractors.
- Parts of the ID extraction utilites required by metadata source plugins
  should live there.
- Also this enables future usage of those utilities from the "outside" of
  metadata source plugins.
- Move Discogs ID extractor to the new module and change test_discogs to use
  the new location.
- Add spotify_id_regex variable to the new module.
@JOJ0
Refactor MetadataSourcePlugin._get_id()
284180e 
and put to use in Spotify plugin.

- Make _get_id() a staticmethod usable from outside a metadata source plugin.
- id_regex now has to be passed as an argument instead of assuming it is
  accessible via an instance variable (self.id_regex).
- In the Spotify plugin, import spotify_id_regex from util.id_extractors
@JOJ0
Move Deezer ID regex to id_extractors module
8ab2569 
in beets.util package.
@JOJ0
Refactor Beatport plugin to use _get_id
f36c55f 
from MetadataSourcePlugin and save beatport_id_regex in id_extractors module.
This streamlines the Beatport release ID extraction magic with plugins Deezer
and Spotify.
@JOJ0
Leave note about Bandcamp IDs in id_extractors module.
aaa4cfc
@JOJ0
Fix Discogs ID extractor to support short format
c48fa0a 
- Often discogs release links used to be written as discogs.com/release/<id>
- Extend one of the existing regex patterns to support that by making the
  trailing dash (-) optional.
- Save a new test regex on regex101.com and update the link to it.
@JOJ0
Copy link
Member Author

JOJ0 commented Mar 8, 2023 •
edited
Loading

I'm merging this now since:

  • I've added additional tests that check whether Spotify, Deezer and Bandcamp URLs and ID's are translated/detected correctly (We've only had a check for the Discogs ID extraction method so far)
  • It currently is a good time since no PR's are "in flight" that seem to touch these parts in the code. There is one old Draft PR touching Discogs things which I will inform now.
  • @sampsyo had agreed a long time ago already that it's a good idea to split the ID extraction tools out to the utils module.
  • I'm taking full responsibility if something breaks but I think we are covered well with tests and this is ready to merge.

@JOJ0 JOJ0 merged commit 40d27f5 into beetbox:master Mar 8, 2023
@arsaboo
Copy link
Contributor

arsaboo commented Mar 8, 2023

This is a much-needed cleanup @JOJ0

Thanks for your efforts and patience on this.

This was referenced Mar 8, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JOJ0 @arsaboo