Fix lastgenre returning empty genre instead of fallback genre #5682
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
JOJ0
changed the title
As reported in #5649 when new last.fm genres were found, they still might get kicked out by the whitelist check in _resolve_genres(). This might lead to _combine_genres() returning an empty list. The desired outcome though is that since still nothing worthwhile was found, the next stage should be entered - which in this case is, returning with the configured fallback genre (or the default fallback None). The any() check makes sure this is the case and moving out the string conversion from _combine_genres() makes this code slightly more readable.
JOJ0
force-pushed
the
fix_lastgenre_5649
branch
from
04f40e7 to
a6c6cca
Compare
JOJ0
force-pushed
the
fix_lastgenre_5649
branch
from
a6c6cca to
72c5450
Compare
| """Combine old and new genres.""" | ||
| self._log.debug(f"fetched last.fm tags: {new}") | ||
| combined = old + new | ||
| resolved = self._resolve_genres(combined) | ||
| return self._to_delimited_genre_string(resolved) or None | ||
| return self._resolve_genres(combined) |
There was a problem hiding this comment.
Changed this to always return what _resolve_genres gives us which is always a list. I think that's more straightforward. What do you think @snejus?
beetsplug/lastgenre/__init__.py
Outdated
| label = f"keep + {label}" | ||
| return self._combine_genres(keep_genres, new_genres), label | ||
| resolved_genres = self._combine_genres(keep_genres, new_genres) | ||
| if any(resolved_genres): |
There was a problem hiding this comment.
I kind of like this since it reads like if we have any worthwhile resolved genres.
The alternative here would be that _combine_genres ensures that no empty string in the list is allowed (another list comprehension in there... :-/), then here a simple if resolved_genres: would suffice.
For me what counts is that reading through _get_genre, since it is the main magic of the plugin, is as comprehensible as possible.
There was a problem hiding this comment.
The alternative here would be that _combine_genres ensures that no empty string in the list is allowed
Seeing that the last list comprehension in _resolve_genres is
valid_tags = [x for x in tags if self._is_valid(x)]Can we exclude empty tags in _is_valid?
There was a problem hiding this comment.
Yes, actually _is_valid does that already! :-) I realised that and removed if any(... again.
There was a problem hiding this comment.
Also note that I moved this list comprehension to a separate function since I reintroduced an original useful behavior of lastgenre: 926c733
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the issue where a last.fm genre was being incorrectly discarded because it didn’t pass the whitelist filter, resulting in an empty genre outcome. The changes update the behavior of genre combination to return a list of resolved genres rather than a delimited string, and adjust the subsequent logic in _get_genre accordingly.
- Change _combine_genres to return a list of genres instead of a string or None.
- Update the genre combination logic in _get_genre to use the resolved genre list and convert it to a delimited string when appropriate.
Comments suppressed due to low confidence (1)
beetsplug/lastgenre/init.py:398
- [nitpick] Consider using 'if resolved_genres:' instead of 'if any(resolved_genres):' for clarity, given that resolved_genres is already a filtered list of genres.
if any(resolved_genres):
- Rename method from _combine_genres() to _combine_resolve_and_log() to make clear that it not only combines new and old genres but also resolves them (which in this plugin's wording means "do the magic" of canonicalizationm, whitelist checking and reducing to a configured genre count). - Clarify docstring. - Add an additional log message telling which existing genres are taken into account BEFORE "the magic happens".
Actually it is kind of weird that _resolve_genres does reduce genres to the configured count while if canonicalization is disabled, this is done somewhere else. This streamlines behaviour: - Remove count reduction from method _to_delimited_genre_string() and rename to reflect what it's actually doing: _format_and_stringify() - Include count reduction at the very end of _resolve_genres() but only in case canonicalization logic hasn't done it already. - Extend the methods docstring with the new behavior.
JOJ0
force-pushed
the
fix_lastgenre_5649
branch
from
fe18241 to
eeb8a13
Compare
Check if for whatever reason (it usually should not happen) empty strings in a list are received. In that case _get_genre should move on to the next stage, which is falling back to the original genre. Also note that even though whitelist is in place, this will happen, resulting in an original genre that might not be in the whitelist. This is expected!
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where the last.fm genre was resolved to an empty string due to whitelist filtering and ensures that a fallback genre or the original genre is returned instead. It also refactors several functions for improved clarity and introduces additional logging to aid debugging.
- Renames _to_delimited_genre_string to _format_and_stringify and updates its tests.
- Refactors _resolve_genres to adjust genre reduction based on configuration.
- Renames _combine_genres to _combine_resolve_and_log, adding logging for existing genres.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/plugins/test_lastgenre.py | Renamed test case and updated expected output for the formatted string function. |
| beetsplug/lastgenre/init.py | Refactored methods with improved docstrings, renamed functions, and revised genre resolution logic. |
_resolve_genres() has made sure of that already which was called by _combine__resolve_and_log().
which was the usual behaviour in lastgenre ever since and it should be kept that way. Also refactor "if track" to use a similar notation for overall code readability.
since it makes sense - the user configured force and wants to overwrite all existing. If nothing's found fallback genre should be set.
- Revert/fix last.fm fetcher methods to validate genres.
- In past versions (<=2.2) _resolve_genres which included whitelist
checks ran instantly after fetching last.fm tags which made sure the
next stage is hit when nothing worthwhile was found (e.g fallback
album -> artist).
- Bring back this behavior but don't run a full _resolve_genres but a
quick valid (whitelist) check only!
- Introduce an extended config/CLI option that allows to really log what
each stage fetches (prior to validation/whitelist filtering).
- Since this potentially is verbose especially with VA albums (a lot
of artist tag fetches) for performance and debug log clutter reasons
this is disabled by default.
- Clarify final last.fm tags debug log message to "valid last.fm genres"
Co-authored-by: Copilot <[email protected]>
Description
Fixes #5649, more precisely the behavior described in comment #5649 (comment) where a last.fm genre was found for the album but
_resolve_genre()kicked it out because it was't allowed by the whitelist which resulted in writing an empty genre as the final result.This fix makes sure an empty genre list (or one with only empty string genres) is not a satisfying outcome and the next stage is hit - returning with either the original genre, the configured fallback genre or None.
Some refactorings to make some of "the magic" in this plugin better readable:
_resolve_genresreturns a list and so does_combine_genres(which is now renamed tocombine_resolve_and_logwhich more precisely reflects what it's doing).format_and_stringify(the count reduction part moved to_resolve_genres)_resolve_genredoes count reduction when canonicalization is configured, it makes sense to also do that in the other case when only whitelist checks are configured._resolve_genregot a massive docstring describing what it's doing - it just does a lot and it deserves some documentation.To Do
Documentation.Changelog.Not required since this bug was introduced in an unreleased PR.