Fixes a bug where existing tags were set to None, if they weren't whitelisted, but an whitelisted canonicalized parent existed up the tree.
In all other cases, the original genres are canonicalized and considered for the final genre, except in the keep_existing logic branch.
This PR fixes the issue and results in the expected behavior for this combination of options.
For the bug to trigger several conditions had to be met:
- Canonicalization is enabled and a whitelist is specified.
- `force` and `keep_existing` are set. Meaning, that Lastfm is queried for a genre, but the existing genres are still left around when none are found online.
- A release with a non-whitelisted genre exists, but that genre has a whitelisted genre parent up the tree.
- That very release has no genre on lastfm.
This is rather convoluted, but stay with me :D
What would happen is the following:
- `keep_genres` is set to the existing genres, as `force` and `keep_existing` is set.
- Genres for `track`/`album`/`artist` aren't found for this release, as they don't exist in lastfm.
- Then the `keep_existing` logic is entered.
- The old logic only checks if the existing genres have an **exact** match for the whitelist. In contrast to all other code branches, we don't do the `_try_resolve_stage` in case there's no direct match, resulting in no match.
- We continue to the fallback logic, which returns the fallback (`None` in my case)
This patch results in one last try to resolve the existing genres when `keep_existing` is set, which includes canonicalization (if enabled).
In case the albumartist genre can't be found (often due to variations of
artist-combination wording issues, eg "featuring", "+", "&" and so on)
use the albumartists list field, fetch a genre for each artist
separately and concatenate them.
Reduce fetcher methods to 3: last.fm can be asked for
for a genre for these combinations of metadata:
- albumartist/album
- artist/track
- artist
Passing them in the callers instead of hiding it in the
methods also helps readability in _get_genre().
Replace the log_and_pretend decorator with a more robust implementation
using singledispatchmethod. This simplifies the genre application logic
by consolidating logging and processing into dedicated methods.
Key changes:
- Remove log_and_pretend decorator in favor of explicit dispatch
- Add _fetch_and_log_genre method to centralize genre fetching and logging
- Log user-configured full object representation instead of specific
attributes
- Introduce _process singledispatchmethod with type-specific handlers
- Use LibModel type hint for broader compatibility
- Simplify command handler by removing duplicate album/item logic
- Replace manual genre application with try_sync for consistency
- Move item and genre apply to separate helper functions. Have one
function for each to not overcomplicate implementation!
- Use a decorator log_and_pretend that logs and does the right thing
depending on wheter --pretend was passed or not.
- Sets --force (internally) automatically if --pretend is given (this is
a behavirol change needing discussion)
experimental, even though a tag last.fm very often returns (in top 20
tag charts!), it is too broad of a term to be pinned downed with any
particular genre, thus can't really be used for canonicalization.
Fixes to the beets default tree and whitlist files I collected over the
years; Includes Tags last.fm returns quite often; Also the
chart.getTopTags API endpoint was checked to make sure the top 100
charts are included in beets default tree and whitelist.
instead of codecs.open(), which most probably is a relict of beets' Python2/3 compatibility area.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
To ensure proper fallback to the next stage, in each stage we do a full
combine/resolve/log.
Also we directly return if have satisfied results. As a bonus this
improves readability.
Some duplicate code on the label magic though...
- Remove "early whitelist check", since it breaks canonicalization of
actually unwanted genres (not whitelisted) resolving "up" to parent
genres.
- Remove the filter_valid_genres method entirely and get back to inline
list comprehensions. The caveat is that None genres are not catched
that way (see below, should be one of the last functions that finally
returns lists only)
- Along the way, fix _last_lookup's rearly return to empty list instead
of None.
This was not thought through clearly before. It now behaves as follows
which I suppose is least surprising to a user:
- force is on, keep_existing is on, but the whitelist is DISABLED
- no stage found anything on last.fm
- fall back to the original genre
If in this example the whitelist would be ENABLED, the behaviour
changes: Only if the existing genre passes the whitelist test the
original is kept.
