feat(vibe): Add Naver VIBE provider#209
Conversation
|
Some albums with a lot of featuring artists for testing: |
kellnerd
left a comment
There was a problem hiding this comment.
Wow, that was fast...
Originally I wanted to continue working on my Discogs provider today, then I decided to give you some quick initial feedback first, but in the end I found a few more things 😅
| override readonly launchDate: PartialDate = { | ||
| year: 2007, | ||
| month: 8, | ||
| }; |
There was a problem hiding this comment.
Should be June 11 2018.
Please be very careful when copy-pasting other provider implementations, it is very easy to introduce mistakes and unused code which makes review/maintenance much harder.
There is a reason I have an empty provider template, although I could probably refine it a bit now that there are more provider implementations which often have the same de-facto standard methods.
There was a problem hiding this comment.
This is actually a bit of a tricky one, since, as the article says, Naver Vibe merged with their second product, Naver Music, in 2020. Naver Music existed much longer than Naver Vibe, specifically since 2002, if this article is to be believed. What's interesting is that the IDs from Naver Music and Naver Vibe are identical, going way back and allowing MusicBrainz to migrate the old links to the Vibe format some time ago: https://tickets.metabrainz.org/browse/MBS-13922
So I'd argue this new provider should have the earlier launch date, even if it's technically not 100% correct for Vibe.
There was a problem hiding this comment.
cc @kellnerd since this is resolved and to make sure you see it.
There was a problem hiding this comment.
I'll un-resolve it for now :)
There was a problem hiding this comment.
Ok, if there are releases on Vibe which could be as old as from 2002, then I agree with using that as the launch date.
P.S. I get notified about all comments in this repo whether you ping me or not 😀
But un-resolving the thread was a good idea, otherwise it is impossible to see this without following a link from an email notification.
There was a problem hiding this comment.
The website appears to have worked a lot differently back then, but for our purposes, I think using 2002 as a launch date is adequate. The (perhaps slightly later?) album URL format still redirects to Vibe: http://music.naver.com/album/index.nhn?albumId=2
Right, sorry about that. I'll keep it in mind for the future. And yes, thanks for un-resolving the thread!
| } | ||
|
|
||
| override getLinkTypesForEntity(): LinkType[] { | ||
| return ['paid streaming', 'paid download']; |
There was a problem hiding this comment.
Is this correct, do they have downloads?
There was a problem hiding this comment.
Yup, though I need to add validation for which tracks do, since all tracks marked as 'overseas' don't
There was a problem hiding this comment.
Is there a way to specify link types for recordings?
Every naver track can be downloaded, but not every album can.
| }) | ||
| } | ||
|
|
||
| private async getTrackArtists(rawTrack: NaverTrack): Promise<ArtistCreditName[]> { |
There was a problem hiding this comment.
It's hard to follow what is happening here... I would like to see some test cases for this function to see what it is supposed to do and if it can be written in a more obvious way. For example, I also have the feeling that we don't have to distinguish between participants and artists in the end and can have one function which converts artists with optional join phrases but I don't feel confident to try without having test coverage.
There was a problem hiding this comment.
Where all other providers (minus Apple and tidal apparently) would provide all participating main track credits, Naver does not and instead moves featuring artists exclusively to the /credits endpoint for tracks
See: https://harmony.pulsewidth.org.uk/release?url=>in=0692788693392®ion=&musicbrainz=&deezer=&itunes=&spotify=&tidal= for the album on other services
The album tracks fetch here: album/34923420/tracks.json omits the featuring artists, so they have to be fetched with track/96143271/credits.json (track/96143271.json returns the same thing as the album tracks fetch). But /credits also returns all artist roles, including non-participating roles which can be seen here
There was a problem hiding this comment.
I should probably make a helper function to generate the join phrases for the artists, since vibe does distinguish between featuring artists and main artists, unlike most other providers
There was a problem hiding this comment.
What the logic is currently doing:
- Check if the track title has an indicator of featuring artists to attempt to save on extra calls; Can be removed if unreliable
if (!trackFeatVariations.some((fv) => rawTrack.trackTitle.toLocaleLowerCase().includes(fv))) { - Fetch track credits
const trackCredits = await this.getRawTrackCredits(rawTrack.trackId); - Filter out featuring artists
const featuringArtists = roles.filter((role) => role.roleName === '피쳐링').flatMap((role) => role.participantList); - Store the ids of the featuring artists
const featuringIds = featuringArtists.map((artist) => artist.id); - Combine and de duplicate all artists
typescript const uniqueArtists = Array.from(new Map(allArtists.map((artist) => [artist.id, artist])).values()); - Use a forEach loop to check the positions of the nearby artists to generate the joinPhrases & map to convertRawParticipant
- Return credits
I want to make a helper function that replaces the job of step 6
|
Quick update (since the weekend has already passed and I found no time for your PRs): Code-wise this is looking good, but I still have to actually test the provider with a few releases myself. |
|
Tested this the other day to add a new release, worked really well! Is there any other tests you'd like me to do? |
|
@Maxr1998 Thanks for looking at it! I still need to make that helper function, and I need to double check to make sure that all tracks on VIBE with featuring artists have one of these strings in the track title. |
|
There is no official checklist for new providers yet, but it would probably be good to have one. |
Got bored and decided to implement VIBE as well...
Might want to take a look at here in particular: https://github.com/kellnerd/harmony/compare/main...Lioncat6:harmony:vibe?expand=1#diff-994ea9806a6ec65de9b9322a6997658f145c4b9a0fc8e88570a249f0a6f34970R145-R258
Closes #208