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

Add Beatport metadata provider #2

Merged
merged 5 commits into from
May 9, 2024
Merged

Add Beatport metadata provider #2

merged 5 commits into from
May 9, 2024

Conversation

atj
Copy link
Collaborator

@atj atj commented May 7, 2024

This has been lightly tested and seems to work OK. It could do with testing against releases with large numbers of tracks, null barcodes etc. Hopefully the code isn't too bad! :)

Once we get to a point where you are happy with the general approach etc. I will add support for searching by UPC.

@atj
Copy link
Collaborator Author

atj commented May 7, 2024

I forgot to mention a couple of things:

  • The permalink in the edit note includes the Beatport release ID only, which then fails because a slug is required.
  • Beatport doesn't support separate mediums so releases like this fail with a "Providers have returned incompatible track lists" error.

Copy link
Owner

@kellnerd kellnerd left a comment

Choose a reason for hiding this comment

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

Thanks, this is looking good already, many parts look like I could have written these myself style-wise.
I haven't tested it yet, but have a few remarks and suggestions nevertheless.

Edit: The solution for the "Providers have returned incompatible track lists" error will simply be to improve the merge function and accept releases with the same total track count as compatible. Maybe reject merges of N mediums into M mediums and only accept 1 medium into M mediums, though.
Definitely doesn't have to be solved inside this PR.

harmonizer/types.ts Outdated Show resolved Hide resolved
providers/Beatport/mod.ts Show resolved Hide resolved
providers/Beatport/mod.ts Outdated Show resolved Hide resolved
providers/Beatport/mod.ts Outdated Show resolved Hide resolved
providers/base.ts Outdated Show resolved Hide resolved
providers/Beatport/json_types.ts Outdated Show resolved Hide resolved
providers/Beatport/json_types.ts Outdated Show resolved Hide resolved
providers/Beatport/json_types.ts Outdated Show resolved Hide resolved
@atj
Copy link
Collaborator Author

atj commented May 8, 2024

Thank you for the helpful comments and suggestions.

Thanks, this is looking good already, many parts look like I could have written these myself style-wise.

I think this in large part down to the Bandcamp provider providing a good example of how to do things along with the general structure and types you've implemented. I'd have been lost without it!

I've dropped the first commit which added the slug property to ReleaseLookupParameters and used your suggested default of "-" which seems to work fine and also means the generated permalinks work as expected.

Copy link
Owner

@kellnerd kellnerd left a comment

Choose a reason for hiding this comment

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

I've finally tested the provider with a few releases, both combined lookups and Beatport-only lookups.
The results look promising, but I have two important (and a few less important) suggestions nevertheless.

I also spotted that I messed up the React code for the catalog number, where the space after the name of the label is missing as JSX does not trat line breaks as HTML whitespace.
You can fix that by putting label and catno into the same line if you want, otherwise I will do it in main.

providers/Beatport/json_types.ts Outdated Show resolved Hide resolved
providers/Beatport/json_types.ts Outdated Show resolved Hide resolved
providers/Beatport/mod.ts Outdated Show resolved Hide resolved
providers/Beatport/mod.ts Outdated Show resolved Hide resolved
providers/Beatport/mod.ts Show resolved Hide resolved
@kellnerd
Copy link
Owner

kellnerd commented May 8, 2024

Oh, and we also have the issue with 100+ track releases here, of course.
If I try the example from that ticket, I get:

Beatport: Failed to find track "https://api-internal.beatportprod.com/v4/catalog/tracks/14868755/" in embedded JSON

First thought: It would be nice to have at least a more specific error for this case.
Second thought: The above message would look better without the quotes around the URL ;)

providers/Beatport/mod.ts Outdated Show resolved Hide resolved
providers/base.ts Outdated Show resolved Hide resolved
server/icons/BrandBeatport.tsx Show resolved Hide resolved
server/icons/BrandBeatport.tsx Outdated Show resolved Hide resolved
server/icons/BrandBeatport.tsx Outdated Show resolved Hide resolved
utils/time.ts Outdated Show resolved Hide resolved
providers/Beatport/mod.ts Outdated Show resolved Hide resolved
providers/Beatport/mod.ts Show resolved Hide resolved
Copy link
Owner

@kellnerd kellnerd left a comment

Choose a reason for hiding this comment

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

Thank you very much, the barcode search is working as expected.

@kellnerd kellnerd merged commit 50d4246 into kellnerd:main May 9, 2024
@kellnerd kellnerd added feature New feature or request provider Metadata provider labels May 29, 2024
@kellnerd kellnerd mentioned this pull request Jun 7, 2024
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request provider Metadata provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants