Skip to content

Add artist_sort as the last entry in the artists to search - #3558

Merged
sampsyo merged 3 commits into
beetbox:masterfrom
hashhar:artist-sort-lyrics
Apr 19, 2020
Merged

Add artist_sort as the last entry in the artists to search#3558
sampsyo merged 3 commits into
beetbox:masterfrom
hashhar:artist-sort-lyrics

Conversation

@hashhar

@hashhar hashhar commented Apr 18, 2020

Copy link
Copy Markdown
Contributor

This will handle cases where the artist name includes some
characters which can cause a search failure for some
websites - mainly unicode.

Fixes #3340.

This will handle cases where the artist name includes some
characters which can cause a search failure for some
websites - mainly unicode.

Fixes #3340.
@hashhar

hashhar commented Apr 18, 2020

Copy link
Copy Markdown
Contributor Author

@sampsyo I've got this working. I was now looking to add tests and could figure out the new cases that will go into LyricsPluginTest.test_search_artist test cases.

But I can see that there are also some integration tests which make actual network requests to verify things but it looks like they are testing the web scraping part (beautiful soup) instead of the actual plugin logic.

What do you think I should do for this change? Is a new test case which verifies that the search_pairs includes the artist_sort is enough? Or should I do something else?

@sampsyo

sampsyo commented Apr 19, 2020

Copy link
Copy Markdown
Member

Nice! Looks great!!

For this, I think just testing search_pairs should be enough. Plugging that in to a mocked web request seems like it doesn't add too much.

hashhar added 2 commits April 19, 2020 19:14
Adjust doc comment to highlight that artist_sort is used as a fallback
@hashhar
hashhar marked this pull request as ready for review April 19, 2020 14:28
@hashhar

hashhar commented Apr 19, 2020

Copy link
Copy Markdown
Contributor Author

@sampsyo This looks good now. Let me know if there are any docs or changelogs that need editing.

sampsyo added a commit that referenced this pull request Apr 19, 2020
@sampsyo
sampsyo merged commit 06bf690 into beetbox:master Apr 19, 2020
@sampsyo

sampsyo commented Apr 19, 2020

Copy link
Copy Markdown
Member

Looks perfect!! Merged with a quick changelog entry. ✨

@hashhar
hashhar deleted the artist-sort-lyrics branch April 19, 2020 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

lyrics: Use MusicBrainz artist aliases if default query is not found

2 participants