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

Use a DOM-injcted file for deezer.com for better data #1735

Merged
merged 4 commits into from Nov 12, 2018

Conversation

lbrueckner
Copy link
Contributor

After I'd adapted the deezer connector for the new layout some days ago I was not very happy with the result, so I have build this new connector which is using the internal (unofficial) deezer api. I believe this is better for various reasons:

I'm using this for a week now and I'm very pleased with the results :-)

I'm not sure if the retry-loop in initConnector() is necessary, it was never executed so far. Maybe the core source is taking care that the page is ready before injecting?

Copy link
Member

@inverse inverse left a comment

Choose a reason for hiding this comment

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

First up - good job - using internal API's can give more info and better stability ;)

Some comments - I'm not a deezer user so have unable to test

}

function lastfmifyArtists(item) {

Copy link
Member

Choose a reason for hiding this comment

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

Extra space

}

function getArtistsnamesByRole(item, roleId) {

Copy link
Member

Choose a reason for hiding this comment

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

Extra spacing

return title;
}

let artists = getArtistsnamesByRole(item, '5'); // featured artists only
Copy link
Member

Choose a reason for hiding this comment

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

Could we define 0 and 5 as variables to give more context.

.filter(removeDublicateArtists);
}

function removeDublicateArtists(artist, index, artists) {
Copy link
Member

Choose a reason for hiding this comment

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

Typo

return item.ART_NAME;
}

return getArtistsList(artists);
Copy link
Member

Choose a reason for hiding this comment

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

Why call this? this returns a list right?

@lbrueckner
Copy link
Contributor Author

Thanks a lot for your feedback!

I've removed the extra newlines, fixed the typo and added two constants for the values used by deezer to seperate main artists from featured artists.

return getArtistsList(artists);

The parameter is an array of artistnames and the return value is a string containing the artistnames in a list, example:

['Kilroy', 'John Doe', 'The Ramblers'] -> 'Kilroy, John Doe & The Ramblers'

And, of course, it would be good to get agreement of other deezer users.

@inverse
Copy link
Member

inverse commented Nov 10, 2018

Any other Deezer uses able to comment?

@lbrueckner
Copy link
Contributor Author

I'm using the injected connector for several thousand scrobbles now with great success. But recently I've found some inconsistencies in the deezer API responses. First I implemented fixes/workarounds, but it was starting to get quite complicated and in the end I think it is not worth the hassle.

My updated code is not longer trying to make sense of the featured artists information in the API response. It is now using the ART_NAME property which is a simple string.

@inverse
Copy link
Member

inverse commented Nov 12, 2018

We can always iterate and your solution already looks much more elegant :)

@inverse inverse merged commit ea7f3ad into web-scrobbler:master Nov 12, 2018
@lbrueckner
Copy link
Contributor Author

Thanks!

@alexesprit alexesprit added connector This issue or pull request is related to connectors feature New feature or request labels Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connector This issue or pull request is related to connectors feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants