-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
I forgot to mention a couple of things:
|
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.
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.
Thank you for the helpful comments and suggestions.
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 |
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'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
.
Oh, and we also have the issue with 100+ track releases here, of course.
First thought: It would be nice to have at least a more specific error for this case. |
MusicBrainz recently switched from using a mix of "duration" and "length" to just "length", so follow suit for consistency.
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.
Thank you very much, the barcode search is working as expected.
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.