Issue2205: search for lyrics using part of song title preceding ':' #2206
Conversation
|
Can you add a changelog entry too? |
| for title in list(titles): | ||
| titles.add(strip_part(title, pattern)) | ||
|
|
||
| # Remove part of the title string after colon ':' |
There was a problem hiding this comment.
This comment could use a tiny bit more detail: in particular, that this helps with songs with subtitles.
|
Nice! That was fast! This looks great to me. And I like the refactoring too. |
Rather than using an unordered set for storing pairs, append to a list and build an OrderedDict from it to filter duplicated strings while keeping order.
| title, artist = item.title, item.artist | ||
| titles = [title] | ||
| artists = [artist] | ||
| titles = set([title]) |
There was a problem hiding this comment.
rather than using set that does not keep order (original title & artist should be yielded first), we should append to a list and remove duplicates at the end.
| title, artist = item.title, item.artist | ||
| titles = [title] | ||
| artists = [artist] | ||
| titles = set([title]) |
There was a problem hiding this comment.
Add a unit test that checks that original artist and title are yielded first.
|
I've changed the implementation slightly, do you mind having another look on it ? |
|
|
||
| # Yield artist and title obtained from item first | ||
| titles = list(OrderedDict.fromkeys([title] + titles)) | ||
| artists = list(OrderedDict.fromkeys([artist] + artists)) |
There was a problem hiding this comment.
Is the idea with the OrderedDict to remove duplicates while preserving the order? That makes sense, but a couple of improvements come to mind:
- First, the original
titleandartistare already first in the list (from the initialization, above). - We could also just avoid inserting duplicates in the first place. Specifically,
strip_partcould return None instead of the original string when the pattern doesn't match, and we could avoid appending that to the list (or just filter out Nones from the list).
There was a problem hiding this comment.
Right. New commit : I moved the burden to update the alternatives lists to the function that performs the re.search and think it works better that way : it avoids the return None+filter processing and makes caller code simpler.
There was a problem hiding this comment.
Nice! This is even better. Using a list of patterns makes this declarative, which avoids repetition and a whole vector of potential bugs. ✨
Thanks! I say merge when ready.
Delegate the update of titles and artists lists to the helper generate_alternatives() function.
No description provided.