-
Notifications
You must be signed in to change notification settings - Fork 18
feat(mora): add provider for mora.jp #168
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
Conversation
kellnerd
left a comment
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! Despite the number of comments, this is looking good already.
I will soon test the provider a bit myself to see where we potentially need more test cases.
P.S. Regarding the CI complaints about formatting, we can fix that by ignoring the testdata folder in the fmt.exclude setting in deno.json.
kellnerd
left a comment
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.
The provider is looking good to me and test coverage is probably sufficient for now.
Is there any other reason why this PR is still marked as draft?
The fact that mora doesn't split off featured artists (and even assigns an artist ID to such collaborations) is probably better handled as part of a general solution (#119).
In future versions we might want to expose artist names and titles in Kana (another use case of #70) and maybe even put bit depth and sampling frequency into an annotation.
I've also noticed that tracks have individual image URLs, but I haven't seen a release with track-specific images. Even if Harmony had an image de-duplication feature, it probably wouldn't be worth it to extract 200px track images.
Nope, just for the tests
Yeah, I checked around too and never found one. mora is a pretty terrible source for images, anyway. |
closes #126
Draft for now, since it could probably use more tests