From 4ff8c34ed10a791d017070a3021871275f8b37a5 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Wed, 3 May 2023 08:28:05 +0200 Subject: [PATCH 01/83] Quickfix lastgenre always overwriting multi-genre --- beetsplug/lastgenre/__init__.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 0498c4c526..19dee3988e 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -250,7 +250,7 @@ def _is_allowed(self, genre): """ if genre is None: return False - if not self.whitelist or genre in self.whitelist: + if not self.whitelist or genre.lower() in self.whitelist: return True return False @@ -315,8 +315,15 @@ def _get_genre(self, obj): """ # Shortcut to existing genre if not forcing. - if not self.config["force"] and self._is_allowed(obj.genre): - return obj.genre, "keep" + if not self.config["force"]: + genres = obj.genre.split(", ") + keep_allowed = [] + for g in genres: + allowed = self._is_allowed(g) + if allowed: + keep_allowed.append(g) + if keep_allowed: + return ", ".join(keep_allowed), "keep" # Track genre (for Items only). if isinstance(obj, library.Item): From 6ab6ae2051b635f94bd8aa83f8514715fb15d621 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Wed, 30 Aug 2023 10:20:34 +0200 Subject: [PATCH 02/83] Fix track-level genre handling in lastgenre plugin When `lastgenre.source: track` is configured, - `lastgenre -a` _should not_ fall back to the album level genre (by making use of the with_album=False kwarg of the Libary's get method). - `lastgenre -a`, when finally storing the genres of _an album_, should _not_ also write the tracks genres (by making use of the inherit=False kwarg of the Album's store method. --- beetsplug/lastgenre/__init__.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 19dee3988e..3e966c3910 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -316,7 +316,10 @@ def _get_genre(self, obj): # Shortcut to existing genre if not forcing. if not self.config["force"]: - genres = obj.genre.split(", ") + if isinstance(obj, library.Item): + genres = obj.get("genre", with_album=False).split(", ") + else: + genres = obj.get("genre").split(", ") keep_allowed = [] for g in genres: allowed = self._is_allowed(g) @@ -420,10 +423,7 @@ def lastgenre_func(lib, opts, args): album, src, ) - if "track" in self.sources: - album.store(inherit=False) - else: - album.store() + album.store(inherit=False) for item in album.items(): # If we're using track-level sources, also look up each From 7d6a4046cef954102a37d7460961b64531f24d28 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sat, 17 Jun 2023 08:42:52 +0200 Subject: [PATCH 03/83] Handle dups of existing genres in lastgenre plugin When handling existing comma-separated genres in the _get_genre method of the plugin, make sure duplicate genres are removed. --- beetsplug/lastgenre/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 3e966c3910..a4124e1a51 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -326,6 +326,7 @@ def _get_genre(self, obj): if allowed: keep_allowed.append(g) if keep_allowed: + keep_allowed = set(keep_allowed) return ", ".join(keep_allowed), "keep" # Track genre (for Items only). From 517c037c2511ba13b1dd468b0f8e136841f03670 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Thu, 16 Nov 2023 13:22:19 +0100 Subject: [PATCH 04/83] Refactor lastgenre keep_allowed to list comprehension --- beetsplug/lastgenre/__init__.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index a4124e1a51..cb7baa8727 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -320,13 +320,8 @@ def _get_genre(self, obj): genres = obj.get("genre", with_album=False).split(", ") else: genres = obj.get("genre").split(", ") - keep_allowed = [] - for g in genres: - allowed = self._is_allowed(g) - if allowed: - keep_allowed.append(g) + keep_allowed = set([g for g in genres if self._is_allowed(g)]) if keep_allowed: - keep_allowed = set(keep_allowed) return ", ".join(keep_allowed), "keep" # Track genre (for Items only). From fe466f4bb3d54ceb57f0b2dd2b33bb5ebe689d96 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Thu, 16 Nov 2023 16:05:16 +0100 Subject: [PATCH 05/83] Use separator as configured instead of hardcoding in lastgenre plugin. --- beetsplug/lastgenre/__init__.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index cb7baa8727..44c9e102a9 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -316,13 +316,14 @@ def _get_genre(self, obj): # Shortcut to existing genre if not forcing. if not self.config["force"]: + separator = self.config["separator"].get() if isinstance(obj, library.Item): - genres = obj.get("genre", with_album=False).split(", ") + genres = obj.get("genre", with_album=False).split(separator) else: - genres = obj.get("genre").split(", ") + genres = obj.get("genre").split(separator) keep_allowed = set([g for g in genres if self._is_allowed(g)]) if keep_allowed: - return ", ".join(keep_allowed), "keep" + return separator.join(keep_allowed), "keep" # Track genre (for Items only). if isinstance(obj, library.Item): From 4ea86507990d75389d993768af625c7ff1a11e49 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Thu, 16 Nov 2023 16:12:33 +0100 Subject: [PATCH 06/83] Use provided deduplicate function for keep_allowed generation, instead of a simple set(). This way we keep the original order of genres. --- beetsplug/lastgenre/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 44c9e102a9..c658812a21 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -321,7 +321,9 @@ def _get_genre(self, obj): genres = obj.get("genre", with_album=False).split(separator) else: genres = obj.get("genre").split(separator) - keep_allowed = set([g for g in genres if self._is_allowed(g)]) + keep_allowed = deduplicate( + [g for g in genres if self._is_allowed(g)] + ) if keep_allowed: return separator.join(keep_allowed), "keep" From 318c02099a9f459014866b809e2e58b4f66c2236 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sat, 14 Sep 2024 23:53:44 +0200 Subject: [PATCH 07/83] Add lastgenre keep_allowed options (-k/-K) - Default to False. - During PR#4982 discussions we came to the conclusion that the following behaviour would be a good new default choice: - Keep whitelisted existing genres - Only Fetch last.fm genres for empty tags. - To get this we also have to change the default of the force option!!! - Resulting in "force: no" and "keep_allowed: yes"; see Case 4 in PR#4982 description - Options are not put to use yet, just defined and defaults set! --- beetsplug/lastgenre/__init__.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index c658812a21..2516de82a4 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -102,7 +102,8 @@ def __init__(self): "fallback": None, "canonical": False, "source": "album", - "force": True, + "force": False, + "keep_allowed": True, "auto": True, "separator": ", ", "prefer_specific": False, @@ -386,6 +387,20 @@ def commands(self): action="store_true", help="re-download genre when already present", ) + lastgenre_cmd.parser.add_option( + "-k", + "--keep-allowed", + dest="keep_allowed", + action="store_true", + help="keep already present genres when whitelisted", + ) + lastgenre_cmd.parser.add_option( + "-K", + "--keep-any", + dest="keep_allowed", + action="store_false", + help="keep any already present genres", + ) lastgenre_cmd.parser.add_option( "-s", "--source", From 40760a062138dab9d5b31ebe6a574e0deb8ea1b5 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sun, 15 Sep 2024 14:19:54 +0200 Subject: [PATCH 08/83] Docs for lastgenre keep_allowed/force Keep both options' "Configuration" chapter texts as compact as possible, while linking to a new chapter that describes all 4 possible combinations in detail. --- docs/plugins/lastgenre.rst | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/docs/plugins/lastgenre.rst b/docs/plugins/lastgenre.rst index 9ba2d4ebaf..2b7300fc95 100644 --- a/docs/plugins/lastgenre.rst +++ b/docs/plugins/lastgenre.rst @@ -111,6 +111,22 @@ Last.fm returns both of those tags, lastgenre is going to use the most popular, which is often the most generic (in this case ``folk``). By setting ``prefer_specific`` to true, lastgenre would use ``americana`` instead. +Handling pre-populated tags +^^^^^^^^^^^^^^^^^^^^^^^^^^^ +The ``force`` and ``keep_allowed`` options control how pre-existing genres are +handled. By default, the plugin fetches new genres for empty tags only +(``force: no``), and keeps whitelisted genres in pre-populated tags +(``keep_allowed: yes``). + +To write new genres to empty tags and keep *any* pre-existing tag content +as-is, set ``force: no`` and ``keep_allowed: no``. + +To *overwrite* any content of pre-populated tags, set ``force: yes`` and +``keep_allowed: no``. + +To *combine* newly fetched last.fm genres with whitelisted pre-existing genres, +set ``force: yes`` and ``keep_allowed: yes``. + Configuration ------------- @@ -128,9 +144,14 @@ configuration file. The available options are: - **fallback**: A string if to use a fallback genre when no genre is found. You can use the empty string ``''`` to reset the genre. Default: None. -- **force**: By default, beets will always fetch new genres, even if the files - already have one. To instead leave genres in place in when they pass the - whitelist, set the ``force`` option to ``no``. +- **force**: By default, lastgenre will fetch new genres for empty tags and + leave pre-existing content in place. When changing this option to ``yes`` + also adjust the ``keep_allowed`` option to your preference (see `Handling + pre-populated tags`_). + Default: ``no``. +- **keep_allowed**: By default, whitelisted genres remain in pre-populated + tags. When changing this optio to ``yes``, also ajdust the ``force`` + option to your preference (see `Handling pre-populated tags`_). Default: ``yes``. - **min_weight**: Minimum popularity factor below which genres are discarded. Default: 10. From d935ec869c28bccac53afef73c582d252a5714c6 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 17 Sep 2024 17:09:28 +0200 Subject: [PATCH 09/83] Implement --force and --keep-allowed behaviours - Retrieving, filtering and deduplicating present genres of Items/Albums via separate methods. - Implement all four cases of behaviour as described in PR#4982 - Issues: - There is quite some unnecessary spliting of genres from strings into lists and the other way round happening throughout the plugin. - In the case where existing genres get "augmented" with last.fm genres, we might end up with _more_ genres than the configured limit. --- beetsplug/lastgenre/__init__.py | 76 +++++++++++++++++++++++++-------- 1 file changed, 58 insertions(+), 18 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 2516de82a4..b558debbe1 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -303,6 +303,25 @@ def fetch_track_genre(self, obj): "track", LASTFM.get_track, obj.artist, obj.title ) + def _get_existing_genres(self, obj, separator): + """Return a list of genres for this Item or Album.""" + if isinstance(obj, library.Item): + item_genre = obj.get("genre", with_album=False).split(separator) + else: + item_genre = obj.get("genre").split(separator) + + if any(item_genre): + return item_genre + return [] + + def _dedup_genres(self, genres, whitelist_only=True): + """Return a list of deduplicated genres. Depending on the + whitelist_only option, gives filtered or unfiltered results.""" + if whitelist_only: + return deduplicate([g for g in genres if self._is_allowed(g)]) + else: + return deduplicate([g for g in genres]) + def _get_genre(self, obj): """Get the genre string for an Album or Item object based on self.sources. Return a `(genre, source)` pair. The @@ -315,30 +334,47 @@ def _get_genre(self, obj): - None """ - # Shortcut to existing genre if not forcing. - if not self.config["force"]: - separator = self.config["separator"].get() - if isinstance(obj, library.Item): - genres = obj.get("genre", with_album=False).split(separator) + separator = self.config["separator"].get() + keep_genres = [] + + if self.config["force"]: + genres = self._get_existing_genres(obj, separator) + # Case 3 - Keep WHITELISTED. Combine with new. + # Case 1 - Keep None. Overwrite all. + if self.config['keep_allowed']: + keep_genres = self._dedup_genres(genres, whitelist_only=True) else: - genres = obj.get("genre").split(separator) - keep_allowed = deduplicate( - [g for g in genres if self._is_allowed(g)] - ) - if keep_allowed: - return separator.join(keep_allowed), "keep" + keep_genres = None + else: + genres = self._get_existing_genres(obj, separator) + # Case 4 - Keep WHITELISTED. Handle empty. + # Case 2 - Keep ANY. Handle empty. + if genres and self.config['keep_allowed']: + keep_genres = self._dedup_genres(genres, whitelist_only=True) + return separator.join(keep_genres), "keep allowed" + elif genres and not self.config['keep_allowed']: + keep_genres = self._dedup_genres(genres) + return separator.join(keep_genres), "keep any" + # else: Move on, genre tag is empty. # Track genre (for Items only). - if isinstance(obj, library.Item): - if "track" in self.sources: - result = self.fetch_track_genre(obj) - if result: - return result, "track" + if isinstance(obj, library.Item) and "track" in self.sources: + result = self.fetch_track_genre(obj) + if result and keep_genres: + results = result.split(separator) + combined_genres = deduplicate(keep_genres + results) + return separator.join(combined_genres), "keep + track" + elif result: + return result, "track" # Album genre. if "album" in self.sources: result = self.fetch_album_genre(obj) - if result: + if result and keep_genres: + results = result.split(separator) + combined_genres = deduplicate(keep_genres + results) + return separator.join(combined_genres), "keep + album" + elif result: return result, "album" # Artist (or album artist) genre. @@ -362,7 +398,11 @@ def _get_genre(self, obj): if item_genres: result, _ = plurality(item_genres) - if result: + if result and keep_genres: + results = result.split(separator) + combined_genres = deduplicate(keep_genres + results) + return separator.join(combined_genres), "keep + artist" + elif result: return result, "artist" # Filter the existing genre. From 11f7a98917ebbcacfe92f98a1453d5adf7a662ec Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Thu, 31 Oct 2024 07:47:00 +0100 Subject: [PATCH 10/83] Refactor keep/new genres combination - Handle genre combination logic in a well documented helper function that also include type hints. - Throughout the _get_genre function rename the result variable to new_genres to make it clearly descriptive. - Rewrite thze _get_genre function's docstring. --- beetsplug/lastgenre/__init__.py | 88 +++++++++++++++++++++------------ 1 file changed, 57 insertions(+), 31 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index b558debbe1..41eab8cb8f 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -322,16 +322,51 @@ def _dedup_genres(self, genres, whitelist_only=True): else: return deduplicate([g for g in genres]) + def _combine_and_label_genres( + self, new_genres: str, keep_genres: list, separator: str, log_label: str + ) -> tuple: + """Combines genres and returns them with a logging label. + + Parameters: + new_genres (str): The new genre string result to process. + keep_genres (list): Existing genres to combine with new ones + separator (str): Separator used to split and join genre strings. + log_label (str): A label (like "track", "album") we possibly + combine with a prefix. For example resulting in something like + "keep + track" or just "track". + + Returns: + tuple: A tuple containing the combined genre string and the "logging + label". + """ + if new_genres and keep_genres: + split_genres = new_genres.split(separator) + combined_genres = deduplicate(keep_genres + split_genres) + return separator.join(combined_genres), f"keep + {log_label}" + elif new_genres: + return new_genres, log_label + return "", log_label + def _get_genre(self, obj): - """Get the genre string for an Album or Item object based on - self.sources. Return a `(genre, source)` pair. The - prioritization order is: + """Get the final genre string for an Album or Item object + + `self.sources` specifies allowed genre sources, prioritized as follows: - track (for Items only) - album - artist - original - fallback - None + + Parameters: + obj: Either an Album or Item object. + + Returns: + tuple: A `(genre, label)` pair, where `label` is a string used for + logging that describes the result. For example, "keep + artist" + indicates that existing genres were combined with new last.fm + genres, while "artist" means only new last.fm genres are + included. """ separator = self.config["separator"].get() @@ -341,7 +376,7 @@ def _get_genre(self, obj): genres = self._get_existing_genres(obj, separator) # Case 3 - Keep WHITELISTED. Combine with new. # Case 1 - Keep None. Overwrite all. - if self.config['keep_allowed']: + if self.config["keep_allowed"]: keep_genres = self._dedup_genres(genres, whitelist_only=True) else: keep_genres = None @@ -349,41 +384,35 @@ def _get_genre(self, obj): genres = self._get_existing_genres(obj, separator) # Case 4 - Keep WHITELISTED. Handle empty. # Case 2 - Keep ANY. Handle empty. - if genres and self.config['keep_allowed']: + if genres and self.config["keep_allowed"]: keep_genres = self._dedup_genres(genres, whitelist_only=True) return separator.join(keep_genres), "keep allowed" - elif genres and not self.config['keep_allowed']: + elif genres and not self.config["keep_allowed"]: keep_genres = self._dedup_genres(genres) return separator.join(keep_genres), "keep any" # else: Move on, genre tag is empty. # Track genre (for Items only). if isinstance(obj, library.Item) and "track" in self.sources: - result = self.fetch_track_genre(obj) - if result and keep_genres: - results = result.split(separator) - combined_genres = deduplicate(keep_genres + results) - return separator.join(combined_genres), "keep + track" - elif result: - return result, "track" + new_genres = self.fetch_track_genre(obj) + return self._combine_and_label_genres( + new_genres, keep_genres, separator, "track" + ) # Album genre. if "album" in self.sources: - result = self.fetch_album_genre(obj) - if result and keep_genres: - results = result.split(separator) - combined_genres = deduplicate(keep_genres + results) - return separator.join(combined_genres), "keep + album" - elif result: - return result, "album" + new_genres = self.fetch_album_genre(obj) + return self._combine_and_label_genres( + new_genres, keep_genres, separator, "album" + ) # Artist (or album artist) genre. if "artist" in self.sources: - result = None + new_genres = None if isinstance(obj, library.Item): - result = self.fetch_artist_genre(obj) + new_genres = self.fetch_artist_genre(obj) elif obj.albumartist != config["va_name"].as_str(): - result = self.fetch_album_artist_genre(obj) + new_genres = self.fetch_album_artist_genre(obj) else: # For "Various Artists", pick the most popular track genre. item_genres = [] @@ -396,14 +425,11 @@ def _get_genre(self, obj): if item_genre: item_genres.append(item_genre) if item_genres: - result, _ = plurality(item_genres) - - if result and keep_genres: - results = result.split(separator) - combined_genres = deduplicate(keep_genres + results) - return separator.join(combined_genres), "keep + artist" - elif result: - return result, "artist" + new_genres, _ = plurality(item_genres) + + return self._combine_and_label_genres( + new_genres, keep_genres, separator, "artist" + ) # Filter the existing genre. if obj.genre: From 2c86af3ce61332504ca74c5c185fc414bb85fc73 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 1 Nov 2024 08:47:28 +0100 Subject: [PATCH 11/83] Quickfix constant msgpack poetry issue --- poetry.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/poetry.lock b/poetry.lock index 8fa603a133..2595d660bd 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1472,7 +1472,7 @@ test = ["pytest", "pytest-cov"] name = "msgpack" version = "1.1.0" description = "MessagePack serializer" -optional = true +optional = false python-versions = ">=3.8" files = [ {file = "msgpack-1.1.0-cp310-cp310-macosx_10_9_universal2.whl", hash = "sha256:7ad442d527a7e358a469faf43fda45aaf4ac3249c8310a82f0ccff9164e5dccd"}, From cbc33e78fc1624b5471dfe410455810f99675768 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 1 Nov 2024 09:45:22 +0100 Subject: [PATCH 12/83] lastgenre: Add comments over groups of methods trying to get a little order in the chaos. Maybe reordering and/or moving out of the main plugin logic would be a better idea for some methods but don't put much more refactoring into this PR to keep it readable. --- beetsplug/lastgenre/__init__.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 41eab8cb8f..4697e73884 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -166,6 +166,8 @@ def sources(self): elif source == "artist": return ("artist",) + # More canonicalization and general helpers. + def _get_depth(self, tag): """Find the depth of a tag in the genres tree.""" depth = None @@ -255,7 +257,7 @@ def _is_allowed(self, genre): return True return False - # Cached entity lookups. + # Cached last.fm entity lookups. def _last_lookup(self, entity, method, *args): """Get a genre based on the named entity using the callable `method` @@ -303,6 +305,8 @@ def fetch_track_genre(self, obj): "track", LASTFM.get_track, obj.artist, obj.title ) + # Main processing: _get_genre() and helpers. + def _get_existing_genres(self, obj, separator): """Return a list of genres for this Item or Album.""" if isinstance(obj, library.Item): @@ -444,6 +448,8 @@ def _get_genre(self, obj): return None, None + # Beets plugin hooks and CLI. + def commands(self): lastgenre_cmd = ui.Subcommand("lastgenre", help="fetch genres") lastgenre_cmd.parser.add_option( From 70d641c556410ca6175ac224fcbfb0fa7cec7597 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sun, 15 Dec 2024 12:36:03 +0100 Subject: [PATCH 13/83] Experiment with test_lastgenre --- test/plugins/test_lastgenre.py | 39 ++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index 17156453ec..5f905e8e53 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -164,6 +164,7 @@ def get_top_tags(self): assert res == ["pop"] def test_get_genre(self): + """All possible (genre, label) pairs""" mock_genres = {"track": "1", "album": "2", "artist": "3"} def mock_fetch_track_genre(self, obj=None): @@ -183,27 +184,53 @@ def mock_fetch_artist_genre(self, obj): item = _common.item() item.genre = mock_genres["track"] - config["lastgenre"] = {"force": False} + # The default setting + config["lastgenre"] = {"force": False, "keep_allowed": True} res = self.plugin._get_genre(item) - assert res == (item.genre, "keep") + assert res == (item.genre, "keep allowed") - config["lastgenre"] = {"force": True, "source": "track"} + # Not forcing and keeping any existing + config["lastgenre"] = {"force": False, "keep_allowed": False} + res = self.plugin._get_genre(item) + assert res == (item.genre, "keep any") + + # Track + config["lastgenre"] = {"force": True, "keep_allowed": False, "source": "track"} res = self.plugin._get_genre(item) assert res == (mock_genres["track"], "track") - config["lastgenre"] = {"source": "album"} + config["lastgenre"] = {"force": True, "keep_allowed": True, "source": "track"} + res = self.plugin._get_genre(item) + assert res == (mock_genres["track"], "keep + track") + print("res after track check:", res) + + # Album + config["lastgenre"] = {"source": "album", "keep_allowed": False} res = self.plugin._get_genre(item) + print("res is:", res) + print("mock_genres is:", mock_genres["album"]) assert res == (mock_genres["album"], "album") - config["lastgenre"] = {"source": "artist"} + config["lastgenre"] = {"source": "album", "keep_allowed": True} + res = self.plugin._get_genre(item) + assert res == (mock_genres["album"], "keep + album") + + # Artist + config["lastgenre"] = {"source": "artist", "keep_allowed": False} res = self.plugin._get_genre(item) assert res == (mock_genres["artist"], "artist") + config["lastgenre"] = {"source": "artist", "keep_allowed": True} + res = self.plugin._get_genre(item) + assert res == (mock_genres["artist"], "keep + artist") + + # Original mock_genres["artist"] = None res = self.plugin._get_genre(item) assert res == (item.genre, "original") - config["lastgenre"] = {"fallback": "rap"} + # Fallback + config["lastgenre"] = {"fallback": "rap", "keep_allowed": False} item.genre = None res = self.plugin._get_genre(item) assert res == (config["lastgenre"]["fallback"].get(), "fallback") From 6866fce36426dd8c1bc4117773b62def201f9edd Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Thu, 2 Jan 2025 12:09:13 +0100 Subject: [PATCH 14/83] Fix default for _dedup_genre whitelist arg when not stated otherwise whitelist_only must be disabled, we assume it that way in _get_genre calls. --- beetsplug/lastgenre/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 4697e73884..1e1b55382b 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -318,7 +318,7 @@ def _get_existing_genres(self, obj, separator): return item_genre return [] - def _dedup_genres(self, genres, whitelist_only=True): + def _dedup_genres(self, genres, whitelist_only=False): """Return a list of deduplicated genres. Depending on the whitelist_only option, gives filtered or unfiltered results.""" if whitelist_only: From 462a7a524a55655fd0009e2d2bc3859b95f9abe0 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Thu, 2 Jan 2025 18:19:26 +0100 Subject: [PATCH 15/83] _combine_and_label return None not empty str --- beetsplug/lastgenre/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 1e1b55382b..9e7904dfd4 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -349,7 +349,7 @@ def _combine_and_label_genres( return separator.join(combined_genres), f"keep + {log_label}" elif new_genres: return new_genres, log_label - return "", log_label + return None, log_label def _get_genre(self, obj): """Get the final genre string for an Album or Item object From b0e0f1b048bd7695e4d0133728abd0d834f6cb90 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Thu, 2 Jan 2025 18:41:09 +0100 Subject: [PATCH 16/83] Fallback to next stage when fetch_ returns None This was the original behaviour and broke when _combine_and_label helper was introduced. --- beetsplug/lastgenre/__init__.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 9e7904dfd4..1236080e60 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -399,16 +399,18 @@ def _get_genre(self, obj): # Track genre (for Items only). if isinstance(obj, library.Item) and "track" in self.sources: new_genres = self.fetch_track_genre(obj) - return self._combine_and_label_genres( - new_genres, keep_genres, separator, "track" - ) + if new_genres: + return self._combine_and_label_genres( + new_genres, keep_genres, separator, "track" + ) # Album genre. if "album" in self.sources: new_genres = self.fetch_album_genre(obj) - return self._combine_and_label_genres( - new_genres, keep_genres, separator, "album" - ) + if new_genres: + return self._combine_and_label_genres( + new_genres, keep_genres, separator, "album" + ) # Artist (or album artist) genre. if "artist" in self.sources: @@ -431,9 +433,10 @@ def _get_genre(self, obj): if item_genres: new_genres, _ = plurality(item_genres) - return self._combine_and_label_genres( - new_genres, keep_genres, separator, "artist" - ) + if new_genres: + return self._combine_and_label_genres( + new_genres, keep_genres, separator, "artist" + ) # Filter the existing genre. if obj.genre: From a434ecfe7b2d61538546dea8bceb5451e00903e6 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Wed, 1 Jan 2025 21:05:54 +0100 Subject: [PATCH 17/83] Refactor _get_genre test to parametrized pytest --- test/plugins/test_lastgenre.py | 322 +++++++++++++++++++++++++-------- 1 file changed, 250 insertions(+), 72 deletions(-) diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index 5f905e8e53..caef1b11ed 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -16,6 +16,8 @@ from unittest.mock import Mock +import pytest + from beets import config from beets.test import _common from beets.test.helper import BeetsTestCase @@ -163,78 +165,6 @@ def get_top_tags(self): res = plugin._tags_for(MockPylastObj(), min_weight=50) assert res == ["pop"] - def test_get_genre(self): - """All possible (genre, label) pairs""" - mock_genres = {"track": "1", "album": "2", "artist": "3"} - - def mock_fetch_track_genre(self, obj=None): - return mock_genres["track"] - - def mock_fetch_album_genre(self, obj): - return mock_genres["album"] - - def mock_fetch_artist_genre(self, obj): - return mock_genres["artist"] - - lastgenre.LastGenrePlugin.fetch_track_genre = mock_fetch_track_genre - lastgenre.LastGenrePlugin.fetch_album_genre = mock_fetch_album_genre - lastgenre.LastGenrePlugin.fetch_artist_genre = mock_fetch_artist_genre - - self._setup_config(whitelist=False) - item = _common.item() - item.genre = mock_genres["track"] - - # The default setting - config["lastgenre"] = {"force": False, "keep_allowed": True} - res = self.plugin._get_genre(item) - assert res == (item.genre, "keep allowed") - - # Not forcing and keeping any existing - config["lastgenre"] = {"force": False, "keep_allowed": False} - res = self.plugin._get_genre(item) - assert res == (item.genre, "keep any") - - # Track - config["lastgenre"] = {"force": True, "keep_allowed": False, "source": "track"} - res = self.plugin._get_genre(item) - assert res == (mock_genres["track"], "track") - - config["lastgenre"] = {"force": True, "keep_allowed": True, "source": "track"} - res = self.plugin._get_genre(item) - assert res == (mock_genres["track"], "keep + track") - print("res after track check:", res) - - # Album - config["lastgenre"] = {"source": "album", "keep_allowed": False} - res = self.plugin._get_genre(item) - print("res is:", res) - print("mock_genres is:", mock_genres["album"]) - assert res == (mock_genres["album"], "album") - - config["lastgenre"] = {"source": "album", "keep_allowed": True} - res = self.plugin._get_genre(item) - assert res == (mock_genres["album"], "keep + album") - - # Artist - config["lastgenre"] = {"source": "artist", "keep_allowed": False} - res = self.plugin._get_genre(item) - assert res == (mock_genres["artist"], "artist") - - config["lastgenre"] = {"source": "artist", "keep_allowed": True} - res = self.plugin._get_genre(item) - assert res == (mock_genres["artist"], "keep + artist") - - # Original - mock_genres["artist"] = None - res = self.plugin._get_genre(item) - assert res == (item.genre, "original") - - # Fallback - config["lastgenre"] = {"fallback": "rap", "keep_allowed": False} - item.genre = None - res = self.plugin._get_genre(item) - assert res == (config["lastgenre"]["fallback"].get(), "fallback") - def test_sort_by_depth(self): self._setup_config(canonical=True) # Normal case. @@ -245,3 +175,251 @@ def test_sort_by_depth(self): tags = ("electronic", "ambient", "chillout") res = self.plugin._sort_by_depth(tags) assert res == ["ambient", "electronic"] + + +@pytest.mark.parametrize( + "config_values, item_genre, mock_genres, expected_result", + [ + # 0 - default setting. keep whitelisted exisiting, new for empty tags. + # (see "Case 4" comment in plugin) + ( + { + "force": False, + "keep_allowed": True, + "source": "album", # means album or artist genre + "whitelist": True, + }, + "allowed genre", + { + "album": "another allowed genre", + }, + ("allowed genre", "keep allowed"), + ), + # 1 - default setting when whitelisted+unknown genre existing + ( + { + "force": False, + "keep_allowed": True, + "source": "album", + "whitelist": True, + }, + "unknown genre, allowed genre", + { + "album": "another allowed genre", + }, + ("allowed genre", "keep allowed"), + ), + # 2 - default setting when only unknown genre existing + # clears existing but does not add new genre. Not desired but expected. + ( + { + "force": False, + "keep_allowed": True, + "source": "album", + "whitelist": True, + }, + "unknown genre", + { + "album": "another allowed genre", + }, + ("", "keep allowed"), + ), + # 3 - default setting on empty tag + ( + { + "force": False, + "keep_allowed": True, + "source": "album", + "whitelist": True, + }, + "", + { + "album": "another allowed genre", + }, + ("another allowed genre", "album"), + ), + # 4 - force and keep whitelisted + # (see "Case 3" comment in plugin) + ( + { + "force": True, + "keep_allowed": True, + "source": "album", + "whitelist": True, + }, + "allowed genre, unknown genre", + { + "album": "another allowed genre", + }, + ("allowed genre, another allowed genre", "keep + album"), + ), + # 5 - force and keep whitelisted. artist genre + ( + { + "force": True, + "keep_allowed": True, + "source": "artist", # means artist genre (only) + "whitelist": True, + }, + "allowed genre, unknown genre", + { + "artist": "another allowed genre", + }, + ("allowed genre, another allowed genre", "keep + artist"), + ), + # 6 - force and keep whitelisted. track genre + ( + { + "force": True, + "keep_allowed": True, + "source": "track", # means track or album or artist genre + "whitelist": True, + }, + "allowed genre, unknown genre", + { + "track": "another allowed genre", + }, + ("allowed genre, another allowed genre", "keep + track"), + ), + # 7 - force and don't keep, overwrites any preexisting + # (see "Case 1" comment in plugin) + ( + { + "force": True, + "keep_allowed": False, + "source": "album", + "whitelist": True, + }, + "allowed genre, unknown genre", + { + "album": "another allowed genre", + }, + ("another allowed genre", "album"), + ), + # 8 - don't force, don't keep allowed - on empty tag + # empty tag gets new genres + # (see "Case 2" comment in plugin) + ( + { + "force": False, + "keep_allowed": False, + "source": "album", + "whitelist": True, + }, + "", + { + "album": "another allowed genre, allowed genre", + }, + ("another allowed genre, allowed genre", "album"), + ), + # 9 - don't force, don't keep allowed - on pre-populated tag + # keeps any preexisting genres + # (see "Case 2" comment in plugin) + ( + { + "force": False, + "keep_allowed": False, + "source": "album", + "whitelist": True, + }, + "any genre", + { + "album": "another allowed genre", + }, + ("any genre", "keep any"), + ), + # 10 - fallback to next stages until found + ( + { + "force": True, + "keep_allowed": True, + "source": "track", + "whitelist": True, + }, + "unknown genre", + { + "track": None, + "album": None, + "artist": "allowed genre", + }, + ("allowed genre", "artist"), + ), + # 11 - fallback to fallback when nothing found + ( + { + "force": True, + "keep_allowed": True, + "source": "track", + "whitelist": True, + "fallback": "fallback genre", + }, + "unknown genre", + { + "track": None, + "album": None, + "artist": None, + }, + ("fallback genre", "fallback"), + ), + # 12 - fallback to allowed pre-existing when nothing found + # runs through _format_tag already, thus capitalized; This happens + # later for fetched genres! + ( + { + "force": True, + "keep_allowed": True, + "source": "album", + "whitelist": True, + }, + "allowed genre", + { + "album": None, + "artist": None, + }, + ("Allowed Genre", "original"), + ), + ], +) +def test_get_genre(config_values, item_genre, mock_genres, expected_result): + """Test _get_genre with various configurations.""" + + def mock_fetch_track_genre(self, obj=None): + return mock_genres["track"] + + def mock_fetch_album_genre(self, obj): + return mock_genres["album"] + + def mock_fetch_artist_genre(self, obj): + return mock_genres["artist"] + + # Mock the last.fm fetchers. When whitelist enabled, we can assume only + # whitelisted genres get returned, the plugin's _resolve_genre method + # ensures it. + lastgenre.LastGenrePlugin.fetch_track_genre = mock_fetch_track_genre + lastgenre.LastGenrePlugin.fetch_album_genre = mock_fetch_album_genre + lastgenre.LastGenrePlugin.fetch_artist_genre = mock_fetch_artist_genre + + # Initialize plugin instance and item + plugin = lastgenre.LastGenrePlugin() + item = _common.item() + item.genre = item_genre + + # Configure + config["lastgenre"] = config_values + + # Mock the whitelist instance variable + plugin.whitelist = ( + set( + [ + "allowed genre", + "also allowed genre", + "another allowed genre", + ] + ) + if config_values.get("whitelist") + else set([]) + ) + + # Run + res = plugin._get_genre(item) + assert res == expected_result From 1c14574e858963fcd22c4a9a4a660d06a194f5cb Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 3 Jan 2025 00:38:10 +0100 Subject: [PATCH 18/83] Add lastgenre testcase with unicode \0 separator --- test/plugins/test_lastgenre.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index caef1b11ed..17107a65c9 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -378,6 +378,21 @@ def test_sort_by_depth(self): }, ("Allowed Genre", "original"), ), + # 13 - test with a null charachter as separator + ( + { + "force": True, + "keep_allowed": True, + "source": "album", + "whitelist": True, + "separator": "\u0000" + }, + "allowed genre", + { + "album": "another allowed genre", + }, + ("allowed genre\u0000another allowed genre", "keep + album"), + ), ], ) def test_get_genre(config_values, item_genre, mock_genres, expected_result): From 90c48ea8cf6a24097d7ff90f75ef6b7570dc8ed7 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 3 Jan 2025 08:34:12 +0100 Subject: [PATCH 19/83] Temporary lastgenre debug messages for @arsaboo --- beetsplug/lastgenre/__init__.py | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 1236080e60..7ad174bbf7 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -190,6 +190,9 @@ def _resolve_genres(self, tags): """Given a list of strings, return a genre by joining them into a single string and (optionally) canonicalizing each. """ + self._log.debug( + f"_resolve_genres received: {tags}", + ) if not tags: return None @@ -228,6 +231,9 @@ def _resolve_genres(self, tags): # c14n only adds allowed genres but we may have had forbidden genres in # the original tags list tags = [self._format_tag(x) for x in tags if self._is_allowed(x)] + self._log.debug( + f"_resolve_genres (canonicalized and whitelist filtered): {tags}", + ) return ( self.config["separator"] @@ -245,7 +251,9 @@ def fetch_genre(self, lastfm_obj): can be found. Ex. 'Electronic, House, Dance' """ min_weight = self.config["min_weight"].get(int) - return self._resolve_genres(self._tags_for(lastfm_obj, min_weight)) + resolved = self._resolve_genres(self._tags_for(lastfm_obj, min_weight)) + self._log.debug(f"fetch_genre returns (whitelist checked): {resolved}") + return resolved def _is_allowed(self, genre): """Determine whether the genre is present in the whitelist, @@ -267,13 +275,16 @@ def _last_lookup(self, entity, method, *args): rough ASCII equivalents in order to return better results from the Last.fm database. """ + self._log.debug( + f"_last_lookup receives: " f"{entity}, {method.__name__}, {args}" + ) # Shortcut if we're missing metadata. if any(not s for s in args): return None key = "{}.{}".format(entity, "-".join(str(a) for a in args)) if key in self._genre_cache: - return self._genre_cache[key] + result = self._genre_cache[key] else: args_replaced = [] for arg in args: @@ -283,7 +294,9 @@ def _last_lookup(self, entity, method, *args): genre = self.fetch_genre(method(*args_replaced)) self._genre_cache[key] = genre - return genre + result = genre + self._log.debug(f"_last_lookup returns: {result}") + return result def fetch_album_genre(self, obj): """Return the album genre for this Item or Album.""" From 4c7d0c98cfadf89a609ef39d374ae58260b14499 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 3 Jan 2025 08:45:52 +0100 Subject: [PATCH 20/83] Clarify lastgenre _is_allowed docstring --- beetsplug/lastgenre/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 7ad174bbf7..2091781988 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -256,8 +256,8 @@ def fetch_genre(self, lastfm_obj): return resolved def _is_allowed(self, genre): - """Determine whether the genre is present in the whitelist, - returning a boolean. + """Returns True if the genre is in the whitelist or the whitelist + feature is disabled. """ if genre is None: return False From 998f2f8984b5b775e157736a68a1708828ba6a90 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 3 Jan 2025 12:39:58 +0100 Subject: [PATCH 21/83] Rewrite docs for keep_allowed/existing change --- docs/plugins/lastgenre.rst | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/docs/plugins/lastgenre.rst b/docs/plugins/lastgenre.rst index 2b7300fc95..186308942f 100644 --- a/docs/plugins/lastgenre.rst +++ b/docs/plugins/lastgenre.rst @@ -113,19 +113,24 @@ popular, which is often the most generic (in this case ``folk``). By setting Handling pre-populated tags ^^^^^^^^^^^^^^^^^^^^^^^^^^^ -The ``force`` and ``keep_allowed`` options control how pre-existing genres are -handled. By default, the plugin fetches new genres for empty tags only -(``force: no``), and keeps whitelisted genres in pre-populated tags -(``keep_allowed: yes``). +The ``force``, ``keep_existing`` and ``whitelist`` options control how +pre-existing genres are handled. -To write new genres to empty tags and keep *any* pre-existing tag content -as-is, set ``force: no`` and ``keep_allowed: no``. +By default, the plugin *combines* newly fetched last.fm genres with whitelisted +pre-existing ones (``force: yes`` and ``keep_existing: yes``). + +To write new genres to empty tags only and keep pre-populated tags untouched, +set ``force: no``. To *overwrite* any content of pre-populated tags, set ``force: yes`` and -``keep_allowed: no``. +``keep_existing: no``. + +To *combine* newly fetched last.fm genres with any pre-existing ones, set +``force: yes``, ``keep_existing: yes`` but also disable the whitelist +(``whitelist: False``). -To *combine* newly fetched last.fm genres with whitelisted pre-existing genres, -set ``force: yes`` and ``keep_allowed: yes``. +Any combinations including ``force: no`` and ``keep_existing: yes`` is invalid +(since _not forcing_ means not touching existing tags anyway). Configuration ------------- @@ -144,14 +149,13 @@ configuration file. The available options are: - **fallback**: A string if to use a fallback genre when no genre is found. You can use the empty string ``''`` to reset the genre. Default: None. -- **force**: By default, lastgenre will fetch new genres for empty tags and - leave pre-existing content in place. When changing this option to ``yes`` - also adjust the ``keep_allowed`` option to your preference (see `Handling - pre-populated tags`_). +- **force**: By default, lastgenre will fetch new genres for empty as well as + pre-populated tags. Adjust the ``keep_existing`` option to combine existing + and new genres. (see `Handling pre-populated tags`_). Default: ``no``. -- **keep_allowed**: By default, whitelisted genres remain in pre-populated - tags. When changing this optio to ``yes``, also ajdust the ``force`` - option to your preference (see `Handling pre-populated tags`_). +- **keep_existing**: By default, genres remain in pre-populated tags. Depending + If a whitelist is in place, existing genres get a cleanup. Only valid in + combination with an active ``force`` option. Default: ``yes``. - **min_weight**: Minimum popularity factor below which genres are discarded. Default: 10. From d0abc0d830c0fc881ceb78b907ca9b8492c69bdc Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 3 Jan 2025 10:04:20 +0100 Subject: [PATCH 22/83] Rename lastgenre option, refactor, new default - Refactor and simplify logic of _get_genre() - Add a config validation function. - New default force: yes, keep_existing: yes (closest to original behaviour) --- beetsplug/lastgenre/__init__.py | 79 ++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 37 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 2091781988..04cdd5905d 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -102,17 +102,28 @@ def __init__(self): "fallback": None, "canonical": False, "source": "album", - "force": False, - "keep_allowed": True, + "force": True, + "keep_existing": True, "auto": True, "separator": ", ", "prefer_specific": False, "title_case": True, } ) - + self.config_validation() self.setup() + def config_validation(self): + """Quits plugin when invalid configurations are detected.""" + keep_existing = self.config["keep_existing"].get() + force = self.config["force"].get() + + if keep_existing and not force: + raise ui.UserError( + "Invalid lastgenre plugin configuration (enable force with " + "keep_existing!)" + ) + def setup(self): """Setup plugin from config options""" if self.config["auto"]: @@ -360,7 +371,7 @@ def _combine_and_label_genres( split_genres = new_genres.split(separator) combined_genres = deduplicate(keep_genres + split_genres) return separator.join(combined_genres), f"keep + {log_label}" - elif new_genres: + if new_genres: return new_genres, log_label return None, log_label @@ -389,38 +400,33 @@ def _get_genre(self, obj): separator = self.config["separator"].get() keep_genres = [] + genres = self._get_existing_genres(obj, separator) + if genres and not self.config["force"]: + # Without force we don't touch pre-populated tags and return early + # with the original contents. We format back to string tough. + keep_genres = self._dedup_genres(genres) + return separator.join(keep_genres), "keep" + if self.config["force"]: - genres = self._get_existing_genres(obj, separator) - # Case 3 - Keep WHITELISTED. Combine with new. - # Case 1 - Keep None. Overwrite all. - if self.config["keep_allowed"]: + # Simply forcing doesn't keep any. + keep_genres = [] + # keep_existing remembers, according to the whitelist setting, + # any or just allowed genres. + if self.config["keep_existing"] and self.config["whitelist"]: keep_genres = self._dedup_genres(genres, whitelist_only=True) - else: - keep_genres = None - else: - genres = self._get_existing_genres(obj, separator) - # Case 4 - Keep WHITELISTED. Handle empty. - # Case 2 - Keep ANY. Handle empty. - if genres and self.config["keep_allowed"]: - keep_genres = self._dedup_genres(genres, whitelist_only=True) - return separator.join(keep_genres), "keep allowed" - elif genres and not self.config["keep_allowed"]: + elif self.config["keep_existing"]: keep_genres = self._dedup_genres(genres) - return separator.join(keep_genres), "keep any" - # else: Move on, genre tag is empty. # Track genre (for Items only). if isinstance(obj, library.Item) and "track" in self.sources: - new_genres = self.fetch_track_genre(obj) - if new_genres: + if new_genres := self.fetch_track_genre(obj): return self._combine_and_label_genres( new_genres, keep_genres, separator, "track" ) # Album genre. if "album" in self.sources: - new_genres = self.fetch_album_genre(obj) - if new_genres: + if new_genres := self.fetch_album_genre(obj): return self._combine_and_label_genres( new_genres, keep_genres, separator, "album" ) @@ -451,17 +457,16 @@ def _get_genre(self, obj): new_genres, keep_genres, separator, "artist" ) - # Filter the existing genre. + # Didn't find anything, leave original if obj.genre: - result = self._resolve_genres([obj.genre]) - if result: - return result, "original" + if result := self._resolve_genres([obj.genre]): + return result, "original, fallback" - # Fallback string. - fallback = self.config["fallback"].get() - if fallback: + # No original, return fallback string + if fallback := self.config["fallback"].get(): return fallback, "fallback" + # No fallback configured return None, None # Beets plugin hooks and CLI. @@ -477,17 +482,17 @@ def commands(self): ) lastgenre_cmd.parser.add_option( "-k", - "--keep-allowed", - dest="keep_allowed", + "--keep-existing", + dest="keep_existing", action="store_true", - help="keep already present genres when whitelisted", + help="keep already present genres", ) lastgenre_cmd.parser.add_option( "-K", - "--keep-any", - dest="keep_allowed", + "--keep-none", + dest="keep_existing", action="store_false", - help="keep any already present genres", + help="don't keep already present genres", ) lastgenre_cmd.parser.add_option( "-s", From 3e452e7b76729d4f7388980acc14b3d3599c52b7 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Fri, 3 Jan 2025 09:34:11 +0100 Subject: [PATCH 23/83] lastgenre test _get_genre for renamed keep_existing and decide to use the original default whitelist instead of trying to mock it. Some of the existing tests do it that way as well. --- test/plugins/test_lastgenre.py | 213 ++++++++++++--------------------- 1 file changed, 78 insertions(+), 135 deletions(-) diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index 17107a65c9..b57f5e5e91 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -180,180 +180,134 @@ def test_sort_by_depth(self): @pytest.mark.parametrize( "config_values, item_genre, mock_genres, expected_result", [ - # 0 - default setting. keep whitelisted exisiting, new for empty tags. - # (see "Case 4" comment in plugin) + # 0 - force and keep whitelisted ( { - "force": False, - "keep_allowed": True, + "force": True, + "keep_existing": True, "source": "album", # means album or artist genre "whitelist": True, + "canonical": False, + "prefer_specific": False, + "count": 10, }, - "allowed genre", - { - "album": "another allowed genre", - }, - ("allowed genre", "keep allowed"), - ), - # 1 - default setting when whitelisted+unknown genre existing - ( - { - "force": False, - "keep_allowed": True, - "source": "album", - "whitelist": True, - }, - "unknown genre, allowed genre", - { - "album": "another allowed genre", - }, - ("allowed genre", "keep allowed"), - ), - # 2 - default setting when only unknown genre existing - # clears existing but does not add new genre. Not desired but expected. - ( - { - "force": False, - "keep_allowed": True, - "source": "album", - "whitelist": True, - }, - "unknown genre", + "Blues", { - "album": "another allowed genre", + "album": "Jazz", }, - ("", "keep allowed"), + ("Blues, Jazz", "keep + album"), ), - # 3 - default setting on empty tag - ( - { - "force": False, - "keep_allowed": True, - "source": "album", - "whitelist": True, - }, - "", - { - "album": "another allowed genre", - }, - ("another allowed genre", "album"), - ), - # 4 - force and keep whitelisted - # (see "Case 3" comment in plugin) + # 1 - force and keep whitelisted, unknown original ( { "force": True, - "keep_allowed": True, + "keep_existing": True, "source": "album", "whitelist": True, + "canonical": False, + "prefer_specific": False, }, - "allowed genre, unknown genre", - { - "album": "another allowed genre", - }, - ("allowed genre, another allowed genre", "keep + album"), - ), - # 5 - force and keep whitelisted. artist genre - ( - { - "force": True, - "keep_allowed": True, - "source": "artist", # means artist genre (only) - "whitelist": True, - }, - "allowed genre, unknown genre", + "original unknown, Blues", { - "artist": "another allowed genre", + "album": "Jazz", }, - ("allowed genre, another allowed genre", "keep + artist"), + ("Blues, Jazz", "keep + album"), ), - # 6 - force and keep whitelisted. track genre + # 2 - force and keep whitelisted on empty tag ( { "force": True, - "keep_allowed": True, - "source": "track", # means track or album or artist genre + "keep_existing": True, + "source": "album", "whitelist": True, + "canonical": False, + "prefer_specific": False, }, - "allowed genre, unknown genre", + "", { - "track": "another allowed genre", + "album": "Jazz", }, - ("allowed genre, another allowed genre", "keep + track"), + ("Jazz", "album"), ), - # 7 - force and don't keep, overwrites any preexisting - # (see "Case 1" comment in plugin) + # 3 force and keep, artist configured ( { "force": True, - "keep_allowed": False, - "source": "album", + "keep_existing": True, + "source": "artist", # means artist genre, original or fallback "whitelist": True, + "canonical": False, + "prefer_specific": False, }, - "allowed genre, unknown genre", + "original unknown, Blues", { - "album": "another allowed genre", + "album": "Jazz", + "artist": "Pop", }, - ("another allowed genre", "album"), + ("Blues, Pop", "keep + artist"), ), - # 8 - don't force, don't keep allowed - on empty tag - # empty tag gets new genres - # (see "Case 2" comment in plugin) + # 4 - don't force, disabled whitelist ( { "force": False, - "keep_allowed": False, + "keep_existing": False, "source": "album", - "whitelist": True, + "whitelist": False, + "canonical": False, + "prefer_specific": False, }, - "", + "any genre", { - "album": "another allowed genre, allowed genre", + "album": "Jazz", }, - ("another allowed genre, allowed genre", "album"), + ("any genre", "keep"), ), - # 9 - don't force, don't keep allowed - on pre-populated tag - # keeps any preexisting genres - # (see "Case 2" comment in plugin) + # 5 - don't force, disabled whitelist, empty ( { "force": False, - "keep_allowed": False, + "keep_existing": False, "source": "album", - "whitelist": True, + "whitelist": False, + "canonical": False, + "prefer_specific": False, }, - "any genre", + "", { - "album": "another allowed genre", + "album": "Jazz", }, - ("any genre", "keep any"), + ("Jazz", "album"), ), - # 10 - fallback to next stages until found + # 6 - fallback to next stages until found ( { "force": True, - "keep_allowed": True, - "source": "track", - "whitelist": True, + "keep_existing": True, + "source": "track", # means track,album,artist,... + "whitelist": False, + "canonical": False, + "prefer_specific": False, }, "unknown genre", { "track": None, "album": None, - "artist": "allowed genre", + "artist": "Jazz", }, - ("allowed genre", "artist"), + ("Unknown Genre, Jazz", "keep + artist"), ), - # 11 - fallback to fallback when nothing found + # 7 - fallback to fallback when nothing found ( { "force": True, - "keep_allowed": True, + "keep_existing": True, "source": "track", "whitelist": True, "fallback": "fallback genre", + "canonical": False, + "prefer_specific": False, }, - "unknown genre", + "original unknown", { "track": None, "album": None, @@ -361,37 +315,39 @@ def test_sort_by_depth(self): }, ("fallback genre", "fallback"), ), - # 12 - fallback to allowed pre-existing when nothing found - # runs through _format_tag already, thus capitalized; This happens - # later for fetched genres! + # 8 - null charachter as separator ( { "force": True, - "keep_allowed": True, + "keep_existing": True, "source": "album", "whitelist": True, + "separator": "\u0000", + "canonical": False, + "prefer_specific": False, }, - "allowed genre", + "Blues", { - "album": None, - "artist": None, + "album": "Jazz", }, - ("Allowed Genre", "original"), + ("Blues\u0000Jazz", "keep + album"), ), - # 13 - test with a null charachter as separator + # 9 - limit a lot of results to just 3 ( { "force": True, - "keep_allowed": True, + "keep_existing": True, "source": "album", "whitelist": True, - "separator": "\u0000" + "count": 3, + "canonical": False, + "prefer_specific": False, }, - "allowed genre", + "original unknown, Blues, Rock, Folk, Metal", { - "album": "another allowed genre", + "album": "Jazz", }, - ("allowed genre\u0000another allowed genre", "keep + album"), + ("Blues, Rock, Metal", "keep + album"), ), ], ) @@ -422,19 +378,6 @@ def mock_fetch_artist_genre(self, obj): # Configure config["lastgenre"] = config_values - # Mock the whitelist instance variable - plugin.whitelist = ( - set( - [ - "allowed genre", - "also allowed genre", - "another allowed genre", - ] - ) - if config_values.get("whitelist") - else set([]) - ) - # Run res = plugin._get_genre(item) assert res == expected_result From 5e6d9170d7b132bf20656a19d89c4f371982d1ce Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sat, 4 Jan 2025 12:20:23 +0100 Subject: [PATCH 24/83] Fix lastgenre "count" issue When original genres were kept (keep_existing option), the final genre count was "off". The reason was that reducing genres to that count is handled in _resolve_genre which wasn't run. - This fixes it by ensuring a run of _resolve_genre in _combine_and_label_genres. - There is a small caveat though: New genres have been run through _resolve_genres already. When they are combined with the old ones, they run through it again. Let's take this into account for now and hope performance doesn't suffer too much. --- beetsplug/lastgenre/__init__.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 04cdd5905d..bebf8c5786 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -370,8 +370,11 @@ def _combine_and_label_genres( if new_genres and keep_genres: split_genres = new_genres.split(separator) combined_genres = deduplicate(keep_genres + split_genres) - return separator.join(combined_genres), f"keep + {log_label}" + # If we combine old and new, run through _resolve_genres (again). + resolved_genres = self._resolve_genres(combined_genres) + return resolved_genres, f"keep + {log_label}" if new_genres: + # New genres ran through _resolve_genres already. return new_genres, log_label return None, log_label From 9f5b4badb276ac70c81070b67cdc9c03f322ed06 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Mon, 6 Jan 2025 00:24:05 +0100 Subject: [PATCH 25/83] Handle genres as list, count/format/str helper - Return fetched genres as a list from _resolve_genres(). - Format, limit to count and join to delimited string in helper function. - Fix docstring. - Leave a couple of temporary debug messages. - Fix original genre fallback - just keep as-is. --- beetsplug/lastgenre/__init__.py | 63 +++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 27 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index bebf8c5786..1e5b6d6332 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -179,6 +179,20 @@ def sources(self): # More canonicalization and general helpers. + def _to_delimited_genre_string(self, tags): + """Reduce tags list to configured count, format and return as delimited + string.""" + separator = self.config["separator"].as_str() + count = self.config["count"].get(int) + self._log.debug(f"Reducing {tags} to configured count {count}") + + genre_string = separator.join( + self._format_tag(tag) for tag in tags[: min(count, len(tags))] + ) + + self._log.debug(f"Reduced and formatted tags to {genre_string}") + return genre_string + def _get_depth(self, tag): """Find the depth of a tag in the genres tree.""" depth = None @@ -198,8 +212,7 @@ def _sort_by_depth(self, tags): return [p[1] for p in depth_tag_pairs] def _resolve_genres(self, tags): - """Given a list of strings, return a genre by joining them into a - single string and (optionally) canonicalizing each. + """Given a list of genre strings, filters, sorts and canonicalizes. """ self._log.debug( f"_resolve_genres received: {tags}", @@ -241,16 +254,12 @@ def _resolve_genres(self, tags): # c14n only adds allowed genres but we may have had forbidden genres in # the original tags list - tags = [self._format_tag(x) for x in tags if self._is_allowed(x)] + tags = [x for x in tags if self._is_allowed(x)] self._log.debug( f"_resolve_genres (canonicalized and whitelist filtered): {tags}", ) - return ( - self.config["separator"] - .as_str() - .join(tags[: self.config["count"].get(int)]) - ) + return tags def _format_tag(self, tag): if self.config["title_case"]: @@ -262,9 +271,9 @@ def fetch_genre(self, lastfm_obj): can be found. Ex. 'Electronic, House, Dance' """ min_weight = self.config["min_weight"].get(int) - resolved = self._resolve_genres(self._tags_for(lastfm_obj, min_weight)) - self._log.debug(f"fetch_genre returns (whitelist checked): {resolved}") - return resolved + fetched = self._tags_for(lastfm_obj, min_weight) + self._log.debug(f"fetch_genre returns (whitelist checked): {fetched}") + return fetched def _is_allowed(self, genre): """Returns True if the genre is in the whitelist or the whitelist @@ -351,31 +360,31 @@ def _dedup_genres(self, genres, whitelist_only=False): return deduplicate([g for g in genres]) def _combine_and_label_genres( - self, new_genres: str, keep_genres: list, separator: str, log_label: str + self, new_genres: list, keep_genres: list, log_label: str ) -> tuple: """Combines genres and returns them with a logging label. Parameters: - new_genres (str): The new genre string result to process. + new_genres (list): The new genre result to process. keep_genres (list): Existing genres to combine with new ones - separator (str): Separator used to split and join genre strings. log_label (str): A label (like "track", "album") we possibly combine with a prefix. For example resulting in something like "keep + track" or just "track". Returns: - tuple: A tuple containing the combined genre string and the "logging - label". + tuple: A tuple containing the combined genre string and the + 'logging label'. """ + self._log.debug(f"_combine got type new_genres: {new_genres}") if new_genres and keep_genres: - split_genres = new_genres.split(separator) - combined_genres = deduplicate(keep_genres + split_genres) - # If we combine old and new, run through _resolve_genres (again). + combined_genres = deduplicate(keep_genres + new_genres) resolved_genres = self._resolve_genres(combined_genres) - return resolved_genres, f"keep + {log_label}" + reduced_genres = self._to_delimited_genre_string(resolved_genres) + return reduced_genres, f"keep + {log_label}" if new_genres: - # New genres ran through _resolve_genres already. - return new_genres, log_label + resolved_genres = self._resolve_genres(new_genres) + reduced_genres = self._to_delimited_genre_string(resolved_genres) + return reduced_genres, log_label return None, log_label def _get_genre(self, obj): @@ -424,14 +433,14 @@ def _get_genre(self, obj): if isinstance(obj, library.Item) and "track" in self.sources: if new_genres := self.fetch_track_genre(obj): return self._combine_and_label_genres( - new_genres, keep_genres, separator, "track" + new_genres, keep_genres, "track" ) # Album genre. if "album" in self.sources: if new_genres := self.fetch_album_genre(obj): return self._combine_and_label_genres( - new_genres, keep_genres, separator, "album" + new_genres, keep_genres, "album" ) # Artist (or album artist) genre. @@ -457,13 +466,12 @@ def _get_genre(self, obj): if new_genres: return self._combine_and_label_genres( - new_genres, keep_genres, separator, "artist" + new_genres, keep_genres, "artist" ) # Didn't find anything, leave original if obj.genre: - if result := self._resolve_genres([obj.genre]): - return result, "original, fallback" + return obj.genre, "original fallback" # No original, return fallback string if fallback := self.config["fallback"].get(): @@ -630,4 +638,5 @@ def _tags_for(self, obj, min_weight=None): # Get strings from tags. res = [el.item.get_name().lower() for el in res] + self._log.debug(f"_tags_for result is: {res}") return res From 50e2619ed2a3d9612bfbb1a985d82cb24e6f9bc0 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Mon, 6 Jan 2025 00:38:01 +0100 Subject: [PATCH 26/83] Refactor lastgenre mocked fetchers return list --- test/plugins/test_lastgenre.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index b57f5e5e91..0c190a60c4 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -193,7 +193,7 @@ def test_sort_by_depth(self): }, "Blues", { - "album": "Jazz", + "album": ["Jazz"], }, ("Blues, Jazz", "keep + album"), ), @@ -209,7 +209,7 @@ def test_sort_by_depth(self): }, "original unknown, Blues", { - "album": "Jazz", + "album": ["Jazz"], }, ("Blues, Jazz", "keep + album"), ), @@ -225,7 +225,7 @@ def test_sort_by_depth(self): }, "", { - "album": "Jazz", + "album": ["Jazz"], }, ("Jazz", "album"), ), @@ -241,8 +241,8 @@ def test_sort_by_depth(self): }, "original unknown, Blues", { - "album": "Jazz", - "artist": "Pop", + "album": ["Jazz"], + "artist": ["Pop"], }, ("Blues, Pop", "keep + artist"), ), @@ -258,7 +258,7 @@ def test_sort_by_depth(self): }, "any genre", { - "album": "Jazz", + "album": ["Jazz"], }, ("any genre", "keep"), ), @@ -274,7 +274,7 @@ def test_sort_by_depth(self): }, "", { - "album": "Jazz", + "album": ["Jazz"], }, ("Jazz", "album"), ), @@ -292,7 +292,7 @@ def test_sort_by_depth(self): { "track": None, "album": None, - "artist": "Jazz", + "artist": ["Jazz"], }, ("Unknown Genre, Jazz", "keep + artist"), ), @@ -328,7 +328,7 @@ def test_sort_by_depth(self): }, "Blues", { - "album": "Jazz", + "album": ["Jazz"], }, ("Blues\u0000Jazz", "keep + album"), ), @@ -345,7 +345,7 @@ def test_sort_by_depth(self): }, "original unknown, Blues, Rock, Folk, Metal", { - "album": "Jazz", + "album": ["Jazz"], }, ("Blues, Rock, Metal", "keep + album"), ), From b4590ec3e0ff805f17af39367fab1098eb05e272 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Mon, 6 Jan 2025 00:53:41 +0100 Subject: [PATCH 27/83] Fix/add lastgenre fallback tests --- test/plugins/test_lastgenre.py | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index 0c190a60c4..4d0a64d49d 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -296,7 +296,7 @@ def test_sort_by_depth(self): }, ("Unknown Genre, Jazz", "keep + artist"), ), - # 7 - fallback to fallback when nothing found + # 7 - fallback to original when nothing found ( { "force": True, @@ -313,9 +313,28 @@ def test_sort_by_depth(self): "album": None, "artist": None, }, + ("original unknown", "original fallback"), + ), + # 8 - fallback to fallback if no original + ( + { + "force": True, + "keep_existing": True, + "source": "track", + "whitelist": True, + "fallback": "fallback genre", + "canonical": False, + "prefer_specific": False, + }, + "", + { + "track": None, + "album": None, + "artist": None, + }, ("fallback genre", "fallback"), ), - # 8 - null charachter as separator + # 9 - null charachter as separator ( { "force": True, @@ -332,7 +351,7 @@ def test_sort_by_depth(self): }, ("Blues\u0000Jazz", "keep + album"), ), - # 9 - limit a lot of results to just 3 + # 10 - limit a lot of results to just 3 ( { "force": True, From 3d036473e7568607513ce88e88869e99a3d7b767 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sun, 5 Jan 2025 11:10:22 +0100 Subject: [PATCH 28/83] lastgenre _is_allowed detailed logging - Add detailed debug logging to learn when and why things go wrong here. - Shorten docstring --- beetsplug/lastgenre/__init__.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 1e5b6d6332..81d290de44 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -276,14 +276,19 @@ def fetch_genre(self, lastfm_obj): return fetched def _is_allowed(self, genre): - """Returns True if the genre is in the whitelist or the whitelist - feature is disabled. + """Returns True if genre in whitelist or whitelist disabled. """ + allowed = False if genre is None: - return False - if not self.whitelist or genre.lower() in self.whitelist: - return True - return False + allowed = False + self._log.debug(f"Genre '{genre}' forbidden. Genre NONE.") + elif not self.whitelist: + allowed = True + self._log.debug(f"Genre '{genre}' allowed. Whitelist OFF.") + elif genre.lower() in self.whitelist: + allowed = True + self._log.debug(f"Genre '{genre}' allowed. FOUND in whitelist.") + return allowed # Cached last.fm entity lookups. From 569ba7301689e12dd2efed998809fbbffb014eb5 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Mon, 6 Jan 2025 23:13:12 +0100 Subject: [PATCH 29/83] Refactor again _combine_and_label_genres --- beetsplug/lastgenre/__init__.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 81d290de44..7cad29a06b 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -381,15 +381,14 @@ def _combine_and_label_genres( 'logging label'. """ self._log.debug(f"_combine got type new_genres: {new_genres}") + combined = deduplicate(keep_genres + new_genres) + resolved = self._resolve_genres(combined) + reduced = self._to_delimited_genre_string(resolved) + if new_genres and keep_genres: - combined_genres = deduplicate(keep_genres + new_genres) - resolved_genres = self._resolve_genres(combined_genres) - reduced_genres = self._to_delimited_genre_string(resolved_genres) - return reduced_genres, f"keep + {log_label}" + return reduced, f"keep + {log_label}" if new_genres: - resolved_genres = self._resolve_genres(new_genres) - reduced_genres = self._to_delimited_genre_string(resolved_genres) - return reduced_genres, log_label + return reduced, log_label return None, log_label def _get_genre(self, obj): From dd3a74ca4d6f30d7bd3eb787a02f4dbb3ca8f490 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Mon, 6 Jan 2025 01:18:45 +0100 Subject: [PATCH 30/83] Fix lastgenre limit to count test - No idea where a missing separator (which is default) could happen...just set it explicitely. - Since we now refactored fetch_genre to returning a list we can add mock multiple fetched gernes easier. --- test/plugins/test_lastgenre.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index 4d0a64d49d..09f1b7b98e 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -351,22 +351,23 @@ def test_sort_by_depth(self): }, ("Blues\u0000Jazz", "keep + album"), ), - # 10 - limit a lot of results to just 3 + # 10 - limit a lot of results ( { "force": True, "keep_existing": True, "source": "album", "whitelist": True, - "count": 3, + "count": 5, "canonical": False, "prefer_specific": False, + "separator": ", ", }, "original unknown, Blues, Rock, Folk, Metal", { - "album": ["Jazz"], + "album": ["Jazz", "Bebop", "Hardbop"], }, - ("Blues, Rock, Metal", "keep + album"), + ("Blues, Rock, Metal, Jazz, Bebop", "keep + album"), ), ], ) From 13ae699230052426b4203d391dd496dea8c1741b Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 7 Jan 2025 02:41:34 +0100 Subject: [PATCH 31/83] Fix _dedup_genre, ensure lower case otherwise deduplicate() can't handle it. --- beetsplug/lastgenre/__init__.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 7cad29a06b..b5e1bc968b 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -358,11 +358,13 @@ def _get_existing_genres(self, obj, separator): def _dedup_genres(self, genres, whitelist_only=False): """Return a list of deduplicated genres. Depending on the - whitelist_only option, gives filtered or unfiltered results.""" + whitelist_only option, gives filtered or unfiltered results. + Makes sure genres are handled all lower case.""" if whitelist_only: - return deduplicate([g for g in genres if self._is_allowed(g)]) - else: - return deduplicate([g for g in genres]) + return deduplicate( + [g.lower() for g in genres if self._is_allowed(g)] + ) + return deduplicate([g.lower() for g in genres]) def _combine_and_label_genres( self, new_genres: list, keep_genres: list, log_label: str From 6cd000750aea48196492f8a0ae9e01f4f81bf5d5 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Mon, 6 Jan 2025 11:23:01 +0100 Subject: [PATCH 32/83] _resolve_genre as list tests, add test_to_delimited_string - Adapt tests to _resolve_genres returning a list with not yet formatted genres. - Rename and adapt test_count -> test_to_delimited_string. Note that the new function does not apply whitelist, prefer anything. It just cuts to count and formats! --- test/plugins/test_lastgenre.py | 58 +++++++++++++++++----------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index 09f1b7b98e..145b038a48 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -46,46 +46,49 @@ def _setup_config( def test_default(self): """Fetch genres with whitelist and c14n deactivated""" self._setup_config() - assert self.plugin._resolve_genres(["delta blues"]) == "Delta Blues" + assert self.plugin._resolve_genres(["delta blues"]) == ["delta blues"] def test_c14n_only(self): """Default c14n tree funnels up to most common genre except for *wrong* genres that stay unchanged. """ self._setup_config(canonical=True, count=99) - assert self.plugin._resolve_genres(["delta blues"]) == "Blues" - assert self.plugin._resolve_genres(["iota blues"]) == "Iota Blues" + assert self.plugin._resolve_genres(["delta blues"]) == ["blues"] + assert self.plugin._resolve_genres(["iota blues"]) == ["iota blues"] def test_whitelist_only(self): """Default whitelist rejects *wrong* (non existing) genres.""" self._setup_config(whitelist=True) - assert self.plugin._resolve_genres(["iota blues"]) == "" + assert self.plugin._resolve_genres(["iota blues"]) == [] def test_whitelist_c14n(self): """Default whitelist and c14n both activated result in all parents genres being selected (from specific to common). """ self._setup_config(canonical=True, whitelist=True, count=99) - assert ( - self.plugin._resolve_genres(["delta blues"]) == "Delta Blues, Blues" - ) + assert self.plugin._resolve_genres(["delta blues"]) == [ + "delta blues", + "blues", + ] def test_whitelist_custom(self): """Keep only genres that are in the whitelist.""" self._setup_config(whitelist={"blues", "rock", "jazz"}, count=2) - assert self.plugin._resolve_genres(["pop", "blues"]) == "Blues" + assert self.plugin._resolve_genres(["pop", "blues"]) == ["blues"] self._setup_config(canonical="", whitelist={"rock"}) - assert self.plugin._resolve_genres(["delta blues"]) == "" + assert self.plugin._resolve_genres(["delta blues"]) == [] - def test_count(self): - """Keep the n first genres, as we expect them to be sorted from more to - less popular. + def test_to_delimited_string(self): + """Keep the n first genres, format them and return a + separator-delimited string. """ - self._setup_config(whitelist={"blues", "rock", "jazz"}, count=2) + self._setup_config(count=2) assert ( - self.plugin._resolve_genres(["jazz", "pop", "rock", "blues"]) - == "Jazz, Rock" + self.plugin._to_delimited_genre_string( + ["jazz", "pop", "rock", "blues"] + ) + == "Jazz, Pop" ) def test_count_c14n(self): @@ -95,31 +98,28 @@ def test_count_c14n(self): ) # thanks to c14n, 'blues' superseeds 'country blues' and takes the # second slot - assert ( - self.plugin._resolve_genres( - ["jazz", "pop", "country blues", "rock"] - ) - == "Jazz, Blues" - ) + assert self.plugin._resolve_genres( + ["jazz", "pop", "country blues", "rock"] + ) == ["jazz", "blues"] def test_c14n_whitelist(self): """Genres first pass through c14n and are then filtered""" self._setup_config(canonical=True, whitelist={"rock"}) - assert self.plugin._resolve_genres(["delta blues"]) == "" + assert self.plugin._resolve_genres(["delta blues"]) == [] def test_empty_string_enables_canonical(self): """For backwards compatibility, setting the `canonical` option to the empty string enables it using the default tree. """ self._setup_config(canonical="", count=99) - assert self.plugin._resolve_genres(["delta blues"]) == "Blues" + assert self.plugin._resolve_genres(["delta blues"]) == ["blues"] def test_empty_string_enables_whitelist(self): """Again for backwards compatibility, setting the `whitelist` option to the empty string enables the default set of genres. """ self._setup_config(whitelist="") - assert self.plugin._resolve_genres(["iota blues"]) == "" + assert self.plugin._resolve_genres(["iota blues"]) == [] def test_prefer_specific_loads_tree(self): """When prefer_specific is enabled but canonical is not the @@ -131,15 +131,15 @@ def test_prefer_specific_loads_tree(self): def test_prefer_specific_without_canonical(self): """Prefer_specific works without canonical.""" self._setup_config(prefer_specific=True, canonical=False, count=4) - assert ( - self.plugin._resolve_genres(["math rock", "post-rock"]) - == "Post-Rock, Math Rock" - ) + assert self.plugin._resolve_genres(["math rock", "post-rock"]) == [ + "post-rock", + "math rock", + ] def test_no_duplicate(self): """Remove duplicated genres.""" self._setup_config(count=99) - assert self.plugin._resolve_genres(["blues", "blues"]) == "Blues" + assert self.plugin._resolve_genres(["blues", "blues"]) == ["blues"] def test_tags_for(self): class MockPylastElem: From 37fc1b6b7fe1d6d845bc6095e4b775c05c5eab7f Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Wed, 8 Jan 2025 18:13:55 +0100 Subject: [PATCH 33/83] Final lastgenre plugin linting --- beetsplug/lastgenre/__init__.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index b5e1bc968b..75336787c2 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -212,8 +212,7 @@ def _sort_by_depth(self, tags): return [p[1] for p in depth_tag_pairs] def _resolve_genres(self, tags): - """Given a list of genre strings, filters, sorts and canonicalizes. - """ + """Given a list of genre strings, filters, sorts and canonicalizes.""" self._log.debug( f"_resolve_genres received: {tags}", ) @@ -276,8 +275,7 @@ def fetch_genre(self, lastfm_obj): return fetched def _is_allowed(self, genre): - """Returns True if genre in whitelist or whitelist disabled. - """ + """Returns True if genre in whitelist or whitelist disabled.""" allowed = False if genre is None: allowed = False From a49f12b1107e7b2691c4a4edbf46e491c960fe7c Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Wed, 8 Jan 2025 18:23:01 +0100 Subject: [PATCH 34/83] Final lastgenre docs changes --- docs/plugins/lastgenre.rst | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/docs/plugins/lastgenre.rst b/docs/plugins/lastgenre.rst index 186308942f..c54d7b1613 100644 --- a/docs/plugins/lastgenre.rst +++ b/docs/plugins/lastgenre.rst @@ -113,6 +113,7 @@ popular, which is often the most generic (in this case ``folk``). By setting Handling pre-populated tags ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + The ``force``, ``keep_existing`` and ``whitelist`` options control how pre-existing genres are handled. @@ -126,11 +127,10 @@ To *overwrite* any content of pre-populated tags, set ``force: yes`` and ``keep_existing: no``. To *combine* newly fetched last.fm genres with any pre-existing ones, set -``force: yes``, ``keep_existing: yes`` but also disable the whitelist -(``whitelist: False``). +``force: yes``, ``keep_existing: yes`` and ``whitelist: False``. -Any combinations including ``force: no`` and ``keep_existing: yes`` is invalid -(since _not forcing_ means not touching existing tags anyway). +Combining ``force: no`` and ``keep_existing: yes`` is invalid (since ``force: +no`` means `not touching` existing tags anyway). Configuration ------------- @@ -150,12 +150,14 @@ configuration file. The available options are: You can use the empty string ``''`` to reset the genre. Default: None. - **force**: By default, lastgenre will fetch new genres for empty as well as - pre-populated tags. Adjust the ``keep_existing`` option to combine existing + pre-populated tags. Enable the ``keep_existing`` option to combine existing and new genres. (see `Handling pre-populated tags`_). Default: ``no``. - **keep_existing**: By default, genres remain in pre-populated tags. Depending - If a whitelist is in place, existing genres get a cleanup. Only valid in - combination with an active ``force`` option. + on whether or not ``whitelist`` is enabled, existing genres get "a cleanup". + Enabling ``keep_existing`` is only valid in combination with an active + ``force`` option. To ensure only fresh last.fm genres, disable this option. + (see `Handling pre-populated tags`_) Default: ``yes``. - **min_weight**: Minimum popularity factor below which genres are discarded. Default: 10. From a7179985875bbaf2f91f473715ae199e25d6dcd4 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Wed, 8 Jan 2025 21:09:27 +0100 Subject: [PATCH 35/83] Add a temporary log around whitelist setup --- beetsplug/lastgenre/__init__.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 75336787c2..8f9da09b91 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -134,6 +134,7 @@ def setup(self): # Read the whitelist file if enabled. self.whitelist = set() wl_filename = self.config["whitelist"].get() + self._log.debug(f"The whitelist config setting is '{wl_filename}'") if wl_filename in (True, ""): # Indicates the default whitelist. wl_filename = WHITELIST if wl_filename: @@ -143,6 +144,9 @@ def setup(self): line = line.decode("utf-8").strip().lower() if line and not line.startswith("#"): self.whitelist.add(line) + self._log.debug( + f"The self.whitelist property after file parsing is '{self.whitelist}'" + ) # Read the genres tree for canonicalization if enabled. self.c14n_branches = [] From da98d37632ff3b5072b8c2bdf6cc4c7812a7b8f1 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Thu, 9 Jan 2025 09:01:44 +0100 Subject: [PATCH 36/83] Temporary debug messages in _sort_by_depth --- beetsplug/lastgenre/__init__.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 8f9da09b91..c9a18089ea 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -204,6 +204,7 @@ def _get_depth(self, tag): if tag in value: depth = value.index(tag) break + self._log.debug("_get_depth returns depth:{}", depth) return depth def _sort_by_depth(self, tags): @@ -213,6 +214,9 @@ def _sort_by_depth(self, tags): depth_tag_pairs = [(self._get_depth(t), t) for t in tags] depth_tag_pairs = [e for e in depth_tag_pairs if e[0] is not None] depth_tag_pairs.sort(reverse=True) + self._log.debug( + "_sort_by_depth sorted depth_tag_paris: {}", depth_tag_pairs + ) return [p[1] for p in depth_tag_pairs] def _resolve_genres(self, tags): From 1aca3989d71192e6b5ce0de0a001b96cbbf9a8ff Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Thu, 9 Jan 2025 09:52:38 +0100 Subject: [PATCH 37/83] Fix previous temp log messages --- beetsplug/lastgenre/__init__.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index c9a18089ea..e46d9fa739 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -145,7 +145,7 @@ def setup(self): if line and not line.startswith("#"): self.whitelist.add(line) self._log.debug( - f"The self.whitelist property after file parsing is '{self.whitelist}'" + "The self.whitelist property after file parsing is '{0}'", self.whitelist ) # Read the genres tree for canonicalization if enabled. @@ -204,7 +204,7 @@ def _get_depth(self, tag): if tag in value: depth = value.index(tag) break - self._log.debug("_get_depth returns depth:{}", depth) + self._log.debug("_get_depth returns depth:{0}", depth) return depth def _sort_by_depth(self, tags): @@ -215,7 +215,7 @@ def _sort_by_depth(self, tags): depth_tag_pairs = [e for e in depth_tag_pairs if e[0] is not None] depth_tag_pairs.sort(reverse=True) self._log.debug( - "_sort_by_depth sorted depth_tag_paris: {}", depth_tag_pairs + "_sort_by_depth sorted depth_tag_paris: {0}", depth_tag_pairs ) return [p[1] for p in depth_tag_pairs] From 847b7260b46b82b2abb2ce83232a2a5d438c2377 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Thu, 9 Jan 2025 22:20:58 +0100 Subject: [PATCH 38/83] Fix most popular track genre fetching (VA albums) --- beetsplug/lastgenre/__init__.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index e46d9fa739..349f060ecd 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -472,9 +472,15 @@ def _get_genre(self, obj): if not item_genre: item_genre = self.fetch_artist_genre(item) if item_genre: - item_genres.append(item_genre) + item_genres += item_genre if item_genres: - new_genres, _ = plurality(item_genres) + most_popular, rank = plurality(item_genres) + new_genres = [most_popular] + self._log.debug( + 'Most popular track genre "{}" ({}) for VA album.', + most_popular, + rank, + ) if new_genres: return self._combine_and_label_genres( From 8d43517a71da0d1866e57356a87cb580bf8d9155 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Thu, 9 Jan 2025 23:07:35 +0100 Subject: [PATCH 39/83] Remove all lastgenre temporary debug logging --- beetsplug/lastgenre/__init__.py | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 349f060ecd..96c493ba1a 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -134,7 +134,6 @@ def setup(self): # Read the whitelist file if enabled. self.whitelist = set() wl_filename = self.config["whitelist"].get() - self._log.debug(f"The whitelist config setting is '{wl_filename}'") if wl_filename in (True, ""): # Indicates the default whitelist. wl_filename = WHITELIST if wl_filename: @@ -144,9 +143,6 @@ def setup(self): line = line.decode("utf-8").strip().lower() if line and not line.startswith("#"): self.whitelist.add(line) - self._log.debug( - "The self.whitelist property after file parsing is '{0}'", self.whitelist - ) # Read the genres tree for canonicalization if enabled. self.c14n_branches = [] @@ -188,13 +184,11 @@ def _to_delimited_genre_string(self, tags): string.""" separator = self.config["separator"].as_str() count = self.config["count"].get(int) - self._log.debug(f"Reducing {tags} to configured count {count}") genre_string = separator.join( self._format_tag(tag) for tag in tags[: min(count, len(tags))] ) - self._log.debug(f"Reduced and formatted tags to {genre_string}") return genre_string def _get_depth(self, tag): @@ -204,7 +198,6 @@ def _get_depth(self, tag): if tag in value: depth = value.index(tag) break - self._log.debug("_get_depth returns depth:{0}", depth) return depth def _sort_by_depth(self, tags): @@ -214,16 +207,10 @@ def _sort_by_depth(self, tags): depth_tag_pairs = [(self._get_depth(t), t) for t in tags] depth_tag_pairs = [e for e in depth_tag_pairs if e[0] is not None] depth_tag_pairs.sort(reverse=True) - self._log.debug( - "_sort_by_depth sorted depth_tag_paris: {0}", depth_tag_pairs - ) return [p[1] for p in depth_tag_pairs] def _resolve_genres(self, tags): """Given a list of genre strings, filters, sorts and canonicalizes.""" - self._log.debug( - f"_resolve_genres received: {tags}", - ) if not tags: return None @@ -262,9 +249,6 @@ def _resolve_genres(self, tags): # c14n only adds allowed genres but we may have had forbidden genres in # the original tags list tags = [x for x in tags if self._is_allowed(x)] - self._log.debug( - f"_resolve_genres (canonicalized and whitelist filtered): {tags}", - ) return tags @@ -279,7 +263,6 @@ def fetch_genre(self, lastfm_obj): """ min_weight = self.config["min_weight"].get(int) fetched = self._tags_for(lastfm_obj, min_weight) - self._log.debug(f"fetch_genre returns (whitelist checked): {fetched}") return fetched def _is_allowed(self, genre): @@ -287,13 +270,10 @@ def _is_allowed(self, genre): allowed = False if genre is None: allowed = False - self._log.debug(f"Genre '{genre}' forbidden. Genre NONE.") elif not self.whitelist: allowed = True - self._log.debug(f"Genre '{genre}' allowed. Whitelist OFF.") elif genre.lower() in self.whitelist: allowed = True - self._log.debug(f"Genre '{genre}' allowed. FOUND in whitelist.") return allowed # Cached last.fm entity lookups. @@ -306,9 +286,6 @@ def _last_lookup(self, entity, method, *args): rough ASCII equivalents in order to return better results from the Last.fm database. """ - self._log.debug( - f"_last_lookup receives: " f"{entity}, {method.__name__}, {args}" - ) # Shortcut if we're missing metadata. if any(not s for s in args): return None @@ -326,7 +303,6 @@ def _last_lookup(self, entity, method, *args): genre = self.fetch_genre(method(*args_replaced)) self._genre_cache[key] = genre result = genre - self._log.debug(f"_last_lookup returns: {result}") return result def fetch_album_genre(self, obj): @@ -656,5 +632,4 @@ def _tags_for(self, obj, min_weight=None): # Get strings from tags. res = [el.item.get_name().lower() for el in res] - self._log.debug(f"_tags_for result is: {res}") return res From 6b41743b9172e93a9f1b982ab0443223fa441f83 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Thu, 9 Jan 2025 23:15:04 +0100 Subject: [PATCH 40/83] Polish 'fetched last.fm tags' debug message The best place to log what we actually fetched from last.fm seems to be here in _combine_and_label_genres. Leave out the existing genres we also receive in this function - less is more. --- beetsplug/lastgenre/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 96c493ba1a..57c0d8238f 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -364,7 +364,7 @@ def _combine_and_label_genres( tuple: A tuple containing the combined genre string and the 'logging label'. """ - self._log.debug(f"_combine got type new_genres: {new_genres}") + self._log.debug(f"fetched last.fm tags: {new_genres}") combined = deduplicate(keep_genres + new_genres) resolved = self._resolve_genres(combined) reduced = self._to_delimited_genre_string(resolved) From 593f5460b68f1ddc426eab1f3c4274a10c28430a Mon Sep 17 00:00:00 2001 From: J0J0 Todos <2733783+JOJ0@users.noreply.github.com> Date: Sat, 11 Jan 2025 09:30:09 +0100 Subject: [PATCH 41/83] Apply type hint suggestions from review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Šarūnas Nejus --- beetsplug/lastgenre/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 57c0d8238f..65172799cc 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -113,7 +113,7 @@ def __init__(self): self.config_validation() self.setup() - def config_validation(self): + def config_validation(self) -> None: """Quits plugin when invalid configurations are detected.""" keep_existing = self.config["keep_existing"].get() force = self.config["force"].get() @@ -179,7 +179,7 @@ def sources(self): # More canonicalization and general helpers. - def _to_delimited_genre_string(self, tags): + def _to_delimited_genre_string(self, tags: list[str]) -> str: """Reduce tags list to configured count, format and return as delimited string.""" separator = self.config["separator"].as_str() From b476560d76b027706e390284d6163bd57a1f3b05 Mon Sep 17 00:00:00 2001 From: J0J0 Todos <2733783+JOJ0@users.noreply.github.com> Date: Sat, 11 Jan 2025 09:36:03 +0100 Subject: [PATCH 42/83] Integrate _format_tag in _to_delimited_... MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit optimize by checking for config once and simplify tags list slicing. Remove _format_tags method. Co-authored-by: Šarūnas Nejus --- beetsplug/lastgenre/__init__.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 65172799cc..b5bfa552ae 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -183,13 +183,13 @@ def _to_delimited_genre_string(self, tags: list[str]) -> str: """Reduce tags list to configured count, format and return as delimited string.""" separator = self.config["separator"].as_str() - count = self.config["count"].get(int) + max_count = self.config["count"].get(int) - genre_string = separator.join( - self._format_tag(tag) for tag in tags[: min(count, len(tags))] - ) + genres = tags[:max_count] + if self.config["title_case"]: + genres = [g.title() for g in genres] - return genre_string + return separator.join(genres) def _get_depth(self, tag): """Find the depth of a tag in the genres tree.""" @@ -252,11 +252,6 @@ def _resolve_genres(self, tags): return tags - def _format_tag(self, tag): - if self.config["title_case"]: - return tag.title() - return tag - def fetch_genre(self, lastfm_obj): """Return the genre for a pylast entity or None if no suitable genre can be found. Ex. 'Electronic, House, Dance' From bd0c02437af491274d25fbcb82deaa3f10ba7269 Mon Sep 17 00:00:00 2001 From: J0J0 Todos <2733783+JOJ0@users.noreply.github.com> Date: Sat, 11 Jan 2025 09:41:44 +0100 Subject: [PATCH 43/83] Apply temp logging leftover review suggestions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Useless variables that only were introduced for temporary debug logging while refactoring earlier. Get rid of them. Co-authored-by: Šarūnas Nejus --- beetsplug/lastgenre/__init__.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index b5bfa552ae..dca09e037d 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -248,17 +248,14 @@ def _resolve_genres(self, tags): # c14n only adds allowed genres but we may have had forbidden genres in # the original tags list - tags = [x for x in tags if self._is_allowed(x)] - - return tags + return [x for x in tags if self._is_allowed(x)] def fetch_genre(self, lastfm_obj): """Return the genre for a pylast entity or None if no suitable genre can be found. Ex. 'Electronic, House, Dance' """ min_weight = self.config["min_weight"].get(int) - fetched = self._tags_for(lastfm_obj, min_weight) - return fetched + return self._tags_for(lastfm_obj, min_weight) def _is_allowed(self, genre): """Returns True if genre in whitelist or whitelist disabled.""" From e1fe6fd3d0cfd673ac2526f5087045d081e7198a Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sat, 4 Nov 2023 00:53:52 +0100 Subject: [PATCH 44/83] Prevent album genre inherit only when source:track is configured. --- beetsplug/lastgenre/__init__.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index dca09e037d..e4a3afe4a3 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -527,7 +527,10 @@ def lastgenre_func(lib, opts, args): album, src, ) - album.store(inherit=False) + if "track" in self.sources: + album.store(inherit=False) + else: + album.store() for item in album.items(): # If we're using track-level sources, also look up each From 14974533e8bf242642223af6f03d292566ca5a9e Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sun, 12 Jan 2025 09:09:20 +0100 Subject: [PATCH 45/83] Revert "Quickfix constant msgpack poetry issue" This reverts commit cfc4c9866bd7be8856b35bfbb62fd778bfd9054e. --- poetry.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/poetry.lock b/poetry.lock index 2595d660bd..8fa603a133 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1472,7 +1472,7 @@ test = ["pytest", "pytest-cov"] name = "msgpack" version = "1.1.0" description = "MessagePack serializer" -optional = false +optional = true python-versions = ">=3.8" files = [ {file = "msgpack-1.1.0-cp310-cp310-macosx_10_9_universal2.whl", hash = "sha256:7ad442d527a7e358a469faf43fda45aaf4ac3249c8310a82f0ccff9164e5dccd"}, From 79b5379dce871c3f8321b8bc3ea672f4a19d7ae1 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sat, 11 Jan 2025 10:44:46 +0100 Subject: [PATCH 46/83] Refactor and rename _is_valid() helper --- beetsplug/lastgenre/__init__.py | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index e4a3afe4a3..c9f19ed30c 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -225,7 +225,7 @@ def _resolve_genres(self, tags): parents = [ x for x in find_parents(tag, self.c14n_branches) - if self._is_allowed(x) + if self._is_valid(x) ] else: parents = [find_parents(tag, self.c14n_branches)[-1]] @@ -248,7 +248,7 @@ def _resolve_genres(self, tags): # c14n only adds allowed genres but we may have had forbidden genres in # the original tags list - return [x for x in tags if self._is_allowed(x)] + return [x for x in tags if self._is_valid(x)] def fetch_genre(self, lastfm_obj): """Return the genre for a pylast entity or None if no suitable genre @@ -257,16 +257,15 @@ def fetch_genre(self, lastfm_obj): min_weight = self.config["min_weight"].get(int) return self._tags_for(lastfm_obj, min_weight) - def _is_allowed(self, genre): - """Returns True if genre in whitelist or whitelist disabled.""" - allowed = False - if genre is None: - allowed = False - elif not self.whitelist: - allowed = True - elif genre.lower() in self.whitelist: - allowed = True - return allowed + def _is_valid(self, genre) -> bool: + """Check if the genre is valid. + + Depending on the whitelist property, valid means a genre is in the + whitelist or any genre is allowed. + """ + if genre and (not self.whitelist or genre.lower() in self.whitelist): + return True + return False # Cached last.fm entity lookups. @@ -336,7 +335,7 @@ def _dedup_genres(self, genres, whitelist_only=False): Makes sure genres are handled all lower case.""" if whitelist_only: return deduplicate( - [g.lower() for g in genres if self._is_allowed(g)] + [g.lower() for g in genres if self._is_valid(g)] ) return deduplicate([g.lower() for g in genres]) From 2a80a10aa82390af7661f15ab0e136073bfa227d Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sun, 12 Jan 2025 17:45:41 +0100 Subject: [PATCH 47/83] Use util.unique_list in fav of deduplicate --- beetsplug/lastgenre/__init__.py | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index c9f19ed30c..d911224b94 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -30,7 +30,7 @@ import yaml from beets import config, library, plugins, ui -from beets.util import normpath, plurality +from beets.util import normpath, plurality, unique_list LASTFM = pylast.LastFMNetwork(api_key=plugins.LASTFM_KEY) @@ -45,15 +45,8 @@ } -def deduplicate(seq): - """Remove duplicates from sequence while preserving order.""" - seen = set() - return [x for x in seq if x not in seen and not seen.add(x)] - - # Canonicalization tree processing. - def flatten_tree(elem, path, branches): """Flatten nested lists/dictionaries into lists of strings (branches). @@ -240,7 +233,7 @@ def _resolve_genres(self, tags): break tags = tags_all - tags = deduplicate(tags) + tags = unique_list(tags) # Sort the tags by specificity. if self.config["prefer_specific"]: @@ -334,10 +327,10 @@ def _dedup_genres(self, genres, whitelist_only=False): whitelist_only option, gives filtered or unfiltered results. Makes sure genres are handled all lower case.""" if whitelist_only: - return deduplicate( + return unique_list( [g.lower() for g in genres if self._is_valid(g)] ) - return deduplicate([g.lower() for g in genres]) + return unique_list([g.lower() for g in genres]) def _combine_and_label_genres( self, new_genres: list, keep_genres: list, log_label: str @@ -356,7 +349,7 @@ def _combine_and_label_genres( 'logging label'. """ self._log.debug(f"fetched last.fm tags: {new_genres}") - combined = deduplicate(keep_genres + new_genres) + combined = unique_list(keep_genres + new_genres) resolved = self._resolve_genres(combined) reduced = self._to_delimited_genre_string(resolved) From d358a24ed9eeaabe3b807bc86fc845641cf64742 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sun, 12 Jan 2025 18:09:53 +0100 Subject: [PATCH 48/83] Remove redundant unique_list call in _combine and clarify in _resolve_genre docstring. --- beetsplug/lastgenre/__init__.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index d911224b94..d024a36174 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -203,7 +203,8 @@ def _sort_by_depth(self, tags): return [p[1] for p in depth_tag_pairs] def _resolve_genres(self, tags): - """Given a list of genre strings, filters, sorts and canonicalizes.""" + """Given a list of genre strings, filters, dedups, sorts and + canonicalizes.""" if not tags: return None @@ -349,7 +350,7 @@ def _combine_and_label_genres( 'logging label'. """ self._log.debug(f"fetched last.fm tags: {new_genres}") - combined = unique_list(keep_genres + new_genres) + combined = keep_genres + new_genres resolved = self._resolve_genres(combined) reduced = self._to_delimited_genre_string(resolved) From f698f21a2802bdb041448a99ba1d3b22d984e964 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sun, 12 Jan 2025 18:35:42 +0100 Subject: [PATCH 49/83] Ensure _resolve returns list, add type hint Prevents potential type erros when handing over to _to_delimited_genre_string. --- beetsplug/lastgenre/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index d024a36174..aebde9b063 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -202,11 +202,11 @@ def _sort_by_depth(self, tags): depth_tag_pairs.sort(reverse=True) return [p[1] for p in depth_tag_pairs] - def _resolve_genres(self, tags): + def _resolve_genres(self, tags) -> list: """Given a list of genre strings, filters, dedups, sorts and canonicalizes.""" if not tags: - return None + return [] count = self.config["count"].get(int) if self.canonicalize: From f16e671ff613c4ea8801c75ea336366f03816142 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Mon, 13 Jan 2025 07:58:46 +0100 Subject: [PATCH 50/83] Simplify _get_genre keep_existing conditional - If the keep_existing option is set, just remember everything for now. - Dedup happening later on via _combine... _resolve_genres... - Even knowing if whitelist or not is not important at this point. --- beetsplug/lastgenre/__init__.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index aebde9b063..248242df1e 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -388,19 +388,18 @@ def _get_genre(self, obj): genres = self._get_existing_genres(obj, separator) if genres and not self.config["force"]: # Without force we don't touch pre-populated tags and return early - # with the original contents. We format back to string tough. + # with the original contents. We deduplicate and format back to + # string though. keep_genres = self._dedup_genres(genres) return separator.join(keep_genres), "keep" if self.config["force"]: # Simply forcing doesn't keep any. keep_genres = [] - # keep_existing remembers, according to the whitelist setting, - # any or just allowed genres. - if self.config["keep_existing"] and self.config["whitelist"]: - keep_genres = self._dedup_genres(genres, whitelist_only=True) - elif self.config["keep_existing"]: - keep_genres = self._dedup_genres(genres) + # Remember existing genres if required. Whitelist validation is + # handled later in _resolve_genres. + if self.config["keep_existing"]: + keep_genres = [g.lower() for g in genres] # Track genre (for Items only). if isinstance(obj, library.Item) and "track" in self.sources: From 4580757c8ec8b204558ae90d751332e671c97868 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sun, 12 Jan 2025 11:07:35 +0100 Subject: [PATCH 51/83] Simplify _last_lookup() f-string, list comprehension, remove redundant vars. --- beetsplug/lastgenre/__init__.py | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 248242df1e..57b60f1cac 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -47,6 +47,7 @@ # Canonicalization tree processing. + def flatten_tree(elem, path, branches): """Flatten nested lists/dictionaries into lists of strings (branches). @@ -266,29 +267,28 @@ def _is_valid(self, genre) -> bool: def _last_lookup(self, entity, method, *args): """Get a genre based on the named entity using the callable `method` whose arguments are given in the sequence `args`. The genre lookup - is cached based on the entity name and the arguments. Before the - lookup, each argument is has some Unicode characters replaced with - rough ASCII equivalents in order to return better results from the + is cached based on the entity name and the arguments. + + Before the lookup, each argument has some Unicode characters replaced + with rough ASCII equivalents in order to return better results from the Last.fm database. """ # Shortcut if we're missing metadata. if any(not s for s in args): return None - key = "{}.{}".format(entity, "-".join(str(a) for a in args)) + key = f"{entity}.{'-'.join(str(a) for a in args)}" + if key in self._genre_cache: - result = self._genre_cache[key] - else: - args_replaced = [] - for arg in args: - for k, v in REPLACE.items(): - arg = arg.replace(k, v) - args_replaced.append(arg) - - genre = self.fetch_genre(method(*args_replaced)) - self._genre_cache[key] = genre - result = genre - return result + return self._genre_cache[key] + + args_replaced = [ + "".join(arg.replace(k, v) for k, v in REPLACE.items()) + for arg in args + ] + self._genre_cache[key] = self.fetch_genre(method(*args_replaced)) + + return self._genre_cache[key] def fetch_album_genre(self, obj): """Return the album genre for this Item or Album.""" From ed68bc019bffcd124e8e86a065cfcf7d45f7d166 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Mon, 13 Jan 2025 22:37:01 +0100 Subject: [PATCH 52/83] Refactor _get_genre, simplify _combine_genre --- beetsplug/lastgenre/__init__.py | 88 +++++++++++++-------------------- 1 file changed, 33 insertions(+), 55 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 57b60f1cac..475c87f745 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -333,34 +333,14 @@ def _dedup_genres(self, genres, whitelist_only=False): ) return unique_list([g.lower() for g in genres]) - def _combine_and_label_genres( - self, new_genres: list, keep_genres: list, log_label: str - ) -> tuple: - """Combines genres and returns them with a logging label. - - Parameters: - new_genres (list): The new genre result to process. - keep_genres (list): Existing genres to combine with new ones - log_label (str): A label (like "track", "album") we possibly - combine with a prefix. For example resulting in something like - "keep + track" or just "track". - - Returns: - tuple: A tuple containing the combined genre string and the - 'logging label'. - """ - self._log.debug(f"fetched last.fm tags: {new_genres}") - combined = keep_genres + new_genres + def _combine_genres(self, old: list[str], new: list[str]) -> str | None: + """Combine old and new genres.""" + self._log.debug(f"fetched last.fm tags: {new}") + combined = old + new resolved = self._resolve_genres(combined) - reduced = self._to_delimited_genre_string(resolved) + return self._to_delimited_genre_string(resolved) or None - if new_genres and keep_genres: - return reduced, f"keep + {log_label}" - if new_genres: - return reduced, log_label - return None, log_label - - def _get_genre(self, obj): + def _get_genre(self, obj) -> str | None: """Get the final genre string for an Album or Item object `self.sources` specifies allowed genre sources, prioritized as follows: @@ -387,41 +367,37 @@ def _get_genre(self, obj): genres = self._get_existing_genres(obj, separator) if genres and not self.config["force"]: - # Without force we don't touch pre-populated tags and return early - # with the original contents. We deduplicate and format back to - # string though. + # Without force pre-populated tags are deduplicated and returned. keep_genres = self._dedup_genres(genres) return separator.join(keep_genres), "keep" if self.config["force"]: - # Simply forcing doesn't keep any. + # Force doesn't keep any unless keep_existing is set. + # Whitelist validation is handled in _resolve_genres. keep_genres = [] - # Remember existing genres if required. Whitelist validation is - # handled later in _resolve_genres. if self.config["keep_existing"]: keep_genres = [g.lower() for g in genres] - # Track genre (for Items only). - if isinstance(obj, library.Item) and "track" in self.sources: - if new_genres := self.fetch_track_genre(obj): - return self._combine_and_label_genres( - new_genres, keep_genres, "track" - ) - - # Album genre. - if "album" in self.sources: - if new_genres := self.fetch_album_genre(obj): - return self._combine_and_label_genres( - new_genres, keep_genres, "album" - ) - - # Artist (or album artist) genre. - if "artist" in self.sources: + # Run through stages: track, album, artist, + # album artist, or most popular track genre. + if ( + isinstance(obj, library.Item) + and "track" in self.sources + and (new_genres := self.fetch_track_genre(obj)) + ): + label = "track" + elif "album" in self.sources and ( + new_genres := self.fetch_album_genre(obj) + ): + label = "album" + elif "artist" in self.sources: new_genres = None if isinstance(obj, library.Item): new_genres = self.fetch_artist_genre(obj) + label = "artist" elif obj.albumartist != config["va_name"].as_str(): new_genres = self.fetch_album_artist_genre(obj) + label = "album artist" else: # For "Various Artists", pick the most popular track genre. item_genres = [] @@ -436,26 +412,28 @@ def _get_genre(self, obj): if item_genres: most_popular, rank = plurality(item_genres) new_genres = [most_popular] + label = "most popular track" self._log.debug( 'Most popular track genre "{}" ({}) for VA album.', most_popular, rank, ) - if new_genres: - return self._combine_and_label_genres( - new_genres, keep_genres, "artist" - ) + # Return with a combined or freshly fetched genre list. + if new_genres: + if keep_genres: + label = f"keep + {label}" + return self._combine_genres(keep_genres, new_genres), label - # Didn't find anything, leave original + # Nothing found, leave original. if obj.genre: return obj.genre, "original fallback" - # No original, return fallback string + # No original, return fallback string. if fallback := self.config["fallback"].get(): return fallback, "fallback" - # No fallback configured + # No fallback configured. return None, None # Beets plugin hooks and CLI. From 6e3f5b3127409dad36679163187c43323a00e5e5 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Mon, 13 Jan 2025 23:06:01 +0100 Subject: [PATCH 53/83] Fix type hints, Refactor existing genres method - Rename method _dedup_genre, since it's only used for finalizing/polishing existing genres. - Return separator-delimited string already. - Decide on not passing "separator" to methods, it's a config setting available throughout the plugin. Assign to variable where useful for readability though. - In the force branch, remove re-assigning keep_genres to empty list. - Fix a test. Existing genres are "polished" now, which means: configured title_case is applied. - Fix/add type hints on all touched and new methods --- beetsplug/lastgenre/__init__.py | 54 +++++++++++++++++++-------------- test/plugins/test_lastgenre.py | 2 +- 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 475c87f745..83f51f3a1a 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -25,11 +25,13 @@ import codecs import os import traceback +from typing import Tuple, Union import pylast import yaml from beets import config, library, plugins, ui +from beets.library import Album, Item from beets.util import normpath, plurality, unique_list LASTFM = pylast.LastFMNetwork(api_key=plugins.LASTFM_KEY) @@ -159,16 +161,16 @@ def setup(self): flatten_tree(genres_tree, [], self.c14n_branches) @property - def sources(self): + def sources(self) -> Tuple[str, ...]: # type: ignore """A tuple of allowed genre sources. May contain 'track', 'album', or 'artist.' """ source = self.config["source"].as_choice(("track", "album", "artist")) if source == "track": return "track", "album", "artist" - elif source == "album": + if source == "album": return "album", "artist" - elif source == "artist": + if source == "artist": return ("artist",) # More canonicalization and general helpers. @@ -203,7 +205,7 @@ def _sort_by_depth(self, tags): depth_tag_pairs.sort(reverse=True) return [p[1] for p in depth_tag_pairs] - def _resolve_genres(self, tags) -> list: + def _resolve_genres(self, tags: list[str]) -> list[str]: """Given a list of genre strings, filters, dedups, sorts and canonicalizes.""" if not tags: @@ -252,7 +254,7 @@ def fetch_genre(self, lastfm_obj): min_weight = self.config["min_weight"].get(int) return self._tags_for(lastfm_obj, min_weight) - def _is_valid(self, genre) -> bool: + def _is_valid(self, genre: str) -> bool: """Check if the genre is valid. Depending on the whitelist property, valid means a genre is in the @@ -312,8 +314,9 @@ def fetch_track_genre(self, obj): # Main processing: _get_genre() and helpers. - def _get_existing_genres(self, obj, separator): + def _get_existing_genres(self, obj: Union[Album, Item]) -> list[str]: """Return a list of genres for this Item or Album.""" + separator = self.config["separator"].get() if isinstance(obj, library.Item): item_genre = obj.get("genre", with_album=False).split(separator) else: @@ -323,24 +326,33 @@ def _get_existing_genres(self, obj, separator): return item_genre return [] - def _dedup_genres(self, genres, whitelist_only=False): - """Return a list of deduplicated genres. Depending on the - whitelist_only option, gives filtered or unfiltered results. - Makes sure genres are handled all lower case.""" - if whitelist_only: - return unique_list( - [g.lower() for g in genres if self._is_valid(g)] - ) - return unique_list([g.lower() for g in genres]) + def _polish_existing_genres(self, genres: list[str]) -> str: + """Return a separator delimited string of deduplicated and formatted + genres. Depending on the whitelist setting, gives filtered or + unfiltered results.""" + separator = self.config["separator"].as_str() + # Ensure querying the config setting, self.whitelist seems to be still + # instanatiated in _get_genre pytest!?! + whitelist = self.config["whitelist"].get() + valid_genres = unique_list( + [g.lower() for g in genres if not whitelist or self._is_valid(g)] + ) + if self.config["title_case"]: + valid_genres = [g.title() for g in valid_genres] + return separator.join(valid_genres) - def _combine_genres(self, old: list[str], new: list[str]) -> str | None: + def _combine_genres( + self, old: list[str], new: list[str] + ) -> Union[str, None]: """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 - def _get_genre(self, obj) -> str | None: + def _get_genre( + self, obj: Union[Album, Item] + ) -> tuple[Union[str, None], ...]: """Get the final genre string for an Album or Item object `self.sources` specifies allowed genre sources, prioritized as follows: @@ -361,20 +373,16 @@ def _get_genre(self, obj) -> str | None: genres, while "artist" means only new last.fm genres are included. """ - - separator = self.config["separator"].get() keep_genres = [] - genres = self._get_existing_genres(obj, separator) + genres = self._get_existing_genres(obj) if genres and not self.config["force"]: # Without force pre-populated tags are deduplicated and returned. - keep_genres = self._dedup_genres(genres) - return separator.join(keep_genres), "keep" + return self._polish_existing_genres(genres), "keep" if self.config["force"]: # Force doesn't keep any unless keep_existing is set. # Whitelist validation is handled in _resolve_genres. - keep_genres = [] if self.config["keep_existing"]: keep_genres = [g.lower() for g in genres] diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index 145b038a48..6ab163deda 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -260,7 +260,7 @@ def test_sort_by_depth(self): { "album": ["Jazz"], }, - ("any genre", "keep"), + ("Any Genre", "keep"), ), # 5 - don't force, disabled whitelist, empty ( From ec10507fab25e9f534acc382ed736a5f74221bb2 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Sun, 19 Jan 2025 15:58:21 +0100 Subject: [PATCH 54/83] Return as-is if no-force --- beetsplug/lastgenre/__init__.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 83f51f3a1a..82134a5aa0 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -374,11 +374,12 @@ def _get_genre( included. """ keep_genres = [] + separator = self.config["separator"].as_str() genres = self._get_existing_genres(obj) if genres and not self.config["force"]: - # Without force pre-populated tags are deduplicated and returned. - return self._polish_existing_genres(genres), "keep" + # Without force pre-populated tags are returned as-is. + return separator.join(genres), "keep" if self.config["force"]: # Force doesn't keep any unless keep_existing is set. From ca5e471f05e5f523a3e29039009d4830a1d15f50 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 07:52:09 +0100 Subject: [PATCH 55/83] Refactor again _last_lookup --- beetsplug/lastgenre/__init__.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 82134a5aa0..425840d226 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -280,15 +280,9 @@ def _last_lookup(self, entity, method, *args): return None key = f"{entity}.{'-'.join(str(a) for a in args)}" - - if key in self._genre_cache: - return self._genre_cache[key] - - args_replaced = [ - "".join(arg.replace(k, v) for k, v in REPLACE.items()) - for arg in args - ] - self._genre_cache[key] = self.fetch_genre(method(*args_replaced)) + if key not in self._genre_cache: + args = [a.replace("\u2010", "-") for a in args] + self._genre_cache[key] = self.fetch_genre(method(*args)) return self._genre_cache[key] From 4e8948d7ca138c2f3b3f1c70475e05db4be0ee09 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 11:36:41 +0100 Subject: [PATCH 56/83] Rework new & refine existing lastgenre docs chapters and describe according to new defaults: force and keep_existing DISABLED, ensuring failsafe operation out of the box. --- docs/plugins/lastgenre.rst | 68 +++++++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 20 deletions(-) diff --git a/docs/plugins/lastgenre.rst b/docs/plugins/lastgenre.rst index c54d7b1613..54e4de7612 100644 --- a/docs/plugins/lastgenre.rst +++ b/docs/plugins/lastgenre.rst @@ -25,7 +25,7 @@ tags can be considered genres. This way, tags like "my favorite music" or "seen live" won't be considered genres. The plugin ships with a fairly extensive `internal whitelist`_, but you can set your own in the config file using the ``whitelist`` configuration value or forgo a whitelist altogether by setting -the option to `false`. +the option to ``no``. The genre list file should contain one genre per line. Blank lines are ignored. For the curious, the default genre list is generated by a `script that scrapes @@ -117,21 +117,47 @@ Handling pre-populated tags The ``force``, ``keep_existing`` and ``whitelist`` options control how pre-existing genres are handled. -By default, the plugin *combines* newly fetched last.fm genres with whitelisted -pre-existing ones (``force: yes`` and ``keep_existing: yes``). +As you would assume, setting ``force: no`` **won't touch pre-existing genre +tags** and will only **fetch new genres for empty tags**. When ``force`` is +``yes`` the setting of the ``whitelist`` option (as documented in `Usage`_) +applies to any existing or newly fetched genres. -To write new genres to empty tags only and keep pre-populated tags untouched, -set ``force: no``. +The follwing configurations are possible: -To *overwrite* any content of pre-populated tags, set ``force: yes`` and -``keep_existing: no``. +**Setup 1** (default) -To *combine* newly fetched last.fm genres with any pre-existing ones, set -``force: yes``, ``keep_existing: yes`` and ``whitelist: False``. +Add new last.fm genres when **empty**. Any present tags stay **untouched**. -Combining ``force: no`` and ``keep_existing: yes`` is invalid (since ``force: +.. code-block:: yaml + + force: no + keep_existing: no + +**Setup 2** + +**Overwrite all**. Only fresh last.fm genres remain. + +.. code-block:: yaml + + force: yes + keep_existing: no + +**Setup 3** + +**Combine** genres in present tags with new ones (be aware of that with an +enabled ``whitelist`` setting, of course some genres might get cleaned up. To +make sure any exisitng genres remain, set ``whitelist: no``). + +.. code-block:: yaml + + force: yes + keep_existing: yes + +If ``force`` is disabled the ``keep_existing`` option is simply ignored (since ``force: no`` means `not touching` existing tags anyway). + + Configuration ------------- @@ -149,20 +175,22 @@ configuration file. The available options are: - **fallback**: A string if to use a fallback genre when no genre is found. You can use the empty string ``''`` to reset the genre. Default: None. -- **force**: By default, lastgenre will fetch new genres for empty as well as - pre-populated tags. Enable the ``keep_existing`` option to combine existing - and new genres. (see `Handling pre-populated tags`_). +- **force**: By default, lastgenre will fetch new genres for empty tags only. + Enable the ``keep_existing`` option to combine existing and new genres. (see + `Handling pre-populated tags`_). + Default: ``no``. +- **keep_existing**: This option alters the ``force`` behaviour. + If both ``force`` and ``keep_existing`` are enabled, existing genres are + combined with new ones. Depending on the ``whitelist`` setting, existing and + new genres are filtered accordingly. To ensure only fresh last.fm genres, + disable this option. (see `Handling pre-populated tags`_) Default: ``no``. -- **keep_existing**: By default, genres remain in pre-populated tags. Depending - on whether or not ``whitelist`` is enabled, existing genres get "a cleanup". - Enabling ``keep_existing`` is only valid in combination with an active - ``force`` option. To ensure only fresh last.fm genres, disable this option. - (see `Handling pre-populated tags`_) - Default: ``yes``. - **min_weight**: Minimum popularity factor below which genres are discarded. Default: 10. - **prefer_specific**: Sort genres by the most to least specific, rather than - most to least popular. Default: ``no``. + most to least popular. Note that this option requires a ``canonical`` tree, + and if not configured it will automatically enable and use the built-in tree. + Default: ``no``. - **source**: Which entity to look up in Last.fm. Can be either ``artist``, ``album`` or ``track``. Default: ``album``. From bb3f9c53c2ddd8a452ead915301375551e09ed45 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 11:41:32 +0100 Subject: [PATCH 57/83] lastgenre new defaults, remove config sanity check --- beetsplug/lastgenre/__init__.py | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 425840d226..00ee125f99 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -98,28 +98,16 @@ def __init__(self): "fallback": None, "canonical": False, "source": "album", - "force": True, - "keep_existing": True, + "force": False, + "keep_existing": False, "auto": True, "separator": ", ", "prefer_specific": False, "title_case": True, } ) - self.config_validation() self.setup() - def config_validation(self) -> None: - """Quits plugin when invalid configurations are detected.""" - keep_existing = self.config["keep_existing"].get() - force = self.config["force"].get() - - if keep_existing and not force: - raise ui.UserError( - "Invalid lastgenre plugin configuration (enable force with " - "keep_existing!)" - ) - def setup(self): """Setup plugin from config options""" if self.config["auto"]: From c3f0abd61ca6ad37168ea1ee7a06a99b295d84ca Mon Sep 17 00:00:00 2001 From: J0J0 Todos <2733783+JOJ0@users.noreply.github.com> Date: Tue, 21 Jan 2025 12:00:49 +0100 Subject: [PATCH 58/83] Fix docstring _resolve_genres MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Šarūnas Nejus --- beetsplug/lastgenre/__init__.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 00ee125f99..07bc3022d9 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -194,8 +194,7 @@ def _sort_by_depth(self, tags): return [p[1] for p in depth_tag_pairs] def _resolve_genres(self, tags: list[str]) -> list[str]: - """Given a list of genre strings, filters, dedups, sorts and - canonicalizes.""" + """Filter, deduplicate, sort and canonicalize the given genres.""" if not tags: return [] From 7ff06df17cfef38c0029b5faeb193d2569220d16 Mon Sep 17 00:00:00 2001 From: J0J0 Todos <2733783+JOJ0@users.noreply.github.com> Date: Tue, 21 Jan 2025 12:05:17 +0100 Subject: [PATCH 59/83] Simplify _get_existing_genres() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Šarūnas Nejus --- beetsplug/lastgenre/__init__.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 07bc3022d9..5da9079961 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -297,15 +297,12 @@ def fetch_track_genre(self, obj): def _get_existing_genres(self, obj: Union[Album, Item]) -> list[str]: """Return a list of genres for this Item or Album.""" - separator = self.config["separator"].get() if isinstance(obj, library.Item): - item_genre = obj.get("genre", with_album=False).split(separator) + genre_str = obj.get("genre", with_album=False) else: - item_genre = obj.get("genre").split(separator) + genre_str = obj.get("genre") - if any(item_genre): - return item_genre - return [] + return genre_str.split(self.config["separator"].get()) def _polish_existing_genres(self, genres: list[str]) -> str: """Return a separator delimited string of deduplicated and formatted From 5d94eb3e138e6ab499232ae83451163613878a98 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 11:52:19 +0100 Subject: [PATCH 60/83] Fix _get_genre docstring --- beetsplug/lastgenre/__init__.py | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 5da9079961..d143ad4a59 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -331,25 +331,23 @@ def _combine_genres( def _get_genre( self, obj: Union[Album, Item] ) -> tuple[Union[str, None], ...]: - """Get the final genre string for an Album or Item object + """Get the final genre string for an Album or Item object. - `self.sources` specifies allowed genre sources, prioritized as follows: + `self.sources` specifies allowed genre sources (track, ablum, artist). + Together with several fallback scenarious, the following stages are run + through: - track (for Items only) - album - - artist - - original - - fallback + - artist, albumartist or (for Various Artists albums) the "most + popular track genre" is used. + - original fallback + - fallback (configured value) - None - Parameters: - obj: Either an Album or Item object. - - Returns: - tuple: A `(genre, label)` pair, where `label` is a string used for - logging that describes the result. For example, "keep + artist" - indicates that existing genres were combined with new last.fm - genres, while "artist" means only new last.fm genres are - included. + A `(genre, label)` pair is returned, where `label` is a string used for + logging that describes the result. For example, "keep + artist" + indicates that existing genres were combined with new last.fm genres, + while "artist" means only new last.fm genres are included. """ keep_genres = [] separator = self.config["separator"].as_str() From 3cc2a5e2c6d3b4dc56fceab2932bb8155db36ef8 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 12:29:40 +0100 Subject: [PATCH 61/83] Fix Tuple with tuple in sources property --- beetsplug/lastgenre/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index d143ad4a59..076fad11b6 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -25,7 +25,7 @@ import codecs import os import traceback -from typing import Tuple, Union +from typing import Union import pylast import yaml @@ -149,7 +149,7 @@ def setup(self): flatten_tree(genres_tree, [], self.c14n_branches) @property - def sources(self) -> Tuple[str, ...]: # type: ignore + def sources(self) -> tuple[str, ...]: """A tuple of allowed genre sources. May contain 'track', 'album', or 'artist.' """ From 169ec20a2f739cb6d4787a4497fc8170f45c1380 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 12:31:15 +0100 Subject: [PATCH 62/83] Remove unused _polish_existing_genres --- beetsplug/lastgenre/__init__.py | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 076fad11b6..fce864c664 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -304,21 +304,6 @@ def _get_existing_genres(self, obj: Union[Album, Item]) -> list[str]: return genre_str.split(self.config["separator"].get()) - def _polish_existing_genres(self, genres: list[str]) -> str: - """Return a separator delimited string of deduplicated and formatted - genres. Depending on the whitelist setting, gives filtered or - unfiltered results.""" - separator = self.config["separator"].as_str() - # Ensure querying the config setting, self.whitelist seems to be still - # instanatiated in _get_genre pytest!?! - whitelist = self.config["whitelist"].get() - valid_genres = unique_list( - [g.lower() for g in genres if not whitelist or self._is_valid(g)] - ) - if self.config["title_case"]: - valid_genres = [g.title() for g in valid_genres] - return separator.join(valid_genres) - def _combine_genres( self, old: list[str], new: list[str] ) -> Union[str, None]: From c9187b40bda3ebdd2244b05a92466094166b3c8e Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 12:38:20 +0100 Subject: [PATCH 63/83] Don't uselessly split/join early returned genres --- beetsplug/lastgenre/__init__.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index fce864c664..d8abd22a12 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -334,15 +334,16 @@ def _get_genre( indicates that existing genres were combined with new last.fm genres, while "artist" means only new last.fm genres are included. """ - keep_genres = [] - separator = self.config["separator"].as_str() - genres = self._get_existing_genres(obj) - if genres and not self.config["force"]: + if not self.config["force"]: # Without force pre-populated tags are returned as-is. - return separator.join(genres), "keep" + if isinstance(obj, library.Item): + return obj.get("genre", with_album=False), "keep, no-force" + return obj.get("genre"), "keep, no-force" if self.config["force"]: + genres = self._get_existing_genres(obj) + keep_genres = [] # Force doesn't keep any unless keep_existing is set. # Whitelist validation is handled in _resolve_genres. if self.config["keep_existing"]: From 6e6a0ad9a9da379732b192f93f4fc2aec4437ec5 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 12:42:24 +0100 Subject: [PATCH 64/83] Return empty tuple instead of disabling type issue --- beetsplug/lastgenre/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index d8abd22a12..3336946d68 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -160,6 +160,7 @@ def sources(self) -> tuple[str, ...]: return "album", "artist" if source == "artist": return ("artist",) + return tuple() # More canonicalization and general helpers. From 8da98e52eed4b1074b933e1953cdcc20c8daaa0f Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 12:46:21 +0100 Subject: [PATCH 65/83] Further clarify lastgenre log-labels state if whitelist was applied or any genre is accepted. --- beetsplug/lastgenre/__init__.py | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 3336946d68..9c9391ef9d 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -335,6 +335,7 @@ def _get_genre( indicates that existing genres were combined with new last.fm genres, while "artist" means only new last.fm genres are included. """ + keep_genres = [] if not self.config["force"]: # Without force pre-populated tags are returned as-is. @@ -344,7 +345,6 @@ def _get_genre( if self.config["force"]: genres = self._get_existing_genres(obj) - keep_genres = [] # Force doesn't keep any unless keep_existing is set. # Whitelist validation is handled in _resolve_genres. if self.config["keep_existing"]: @@ -357,19 +357,23 @@ def _get_genre( and "track" in self.sources and (new_genres := self.fetch_track_genre(obj)) ): - label = "track" + label = "track, whitelist" if self.whitelist else "track, any" elif "album" in self.sources and ( new_genres := self.fetch_album_genre(obj) ): - label = "album" + label = "album, whitelist" if self.whitelist else "album, any" elif "artist" in self.sources: new_genres = None if isinstance(obj, library.Item): new_genres = self.fetch_artist_genre(obj) - label = "artist" + label = "artist, whitelist" if self.whitelist else "artist, any" elif obj.albumartist != config["va_name"].as_str(): new_genres = self.fetch_album_artist_genre(obj) - label = "album artist" + label = ( + "album artist, whitelist" + if self.whitelist + else "album artist, any" + ) else: # For "Various Artists", pick the most popular track genre. item_genres = [] @@ -384,7 +388,11 @@ def _get_genre( if item_genres: most_popular, rank = plurality(item_genres) new_genres = [most_popular] - label = "most popular track" + label = ( + "most popular track, whitelist" + if self.whitelist + else "most popular track, any" + ) self._log.debug( 'Most popular track genre "{}" ({}) for VA album.', most_popular, From d5cf376a5142f974d8a6ea3b282a53a3619a8cf5 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 12:58:18 +0100 Subject: [PATCH 66/83] Include lower-casing in _get_existing already since we don't use it for early-returning no-force-existing genres anymore. --- beetsplug/lastgenre/__init__.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 9c9391ef9d..62432055e1 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -303,7 +303,9 @@ def _get_existing_genres(self, obj: Union[Album, Item]) -> list[str]: else: genre_str = obj.get("genre") - return genre_str.split(self.config["separator"].get()) + return [ + g.lower() for g in genre_str.split(self.config["separator"].get()) + ] def _combine_genres( self, old: list[str], new: list[str] @@ -344,11 +346,10 @@ def _get_genre( return obj.get("genre"), "keep, no-force" if self.config["force"]: - genres = self._get_existing_genres(obj) # Force doesn't keep any unless keep_existing is set. # Whitelist validation is handled in _resolve_genres. if self.config["keep_existing"]: - keep_genres = [g.lower() for g in genres] + keep_genres = self._get_existing_genres(obj) # Run through stages: track, album, artist, # album artist, or most popular track genre. From 44901873f7dcdd9550034357e96cf5f76275c532 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 13:40:29 +0100 Subject: [PATCH 67/83] Clarify log-label: keep any, no-force --- beetsplug/lastgenre/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 62432055e1..573fd6dca4 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -342,8 +342,8 @@ def _get_genre( if not self.config["force"]: # Without force pre-populated tags are returned as-is. if isinstance(obj, library.Item): - return obj.get("genre", with_album=False), "keep, no-force" - return obj.get("genre"), "keep, no-force" + return obj.get("genre", with_album=False), "keep any, no-force" + return obj.get("genre"), "keep any, no-force" if self.config["force"]: # Force doesn't keep any unless keep_existing is set. From d703a7f712c54fcb7b80da28dc9f835904c25eef Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 17:14:15 +0100 Subject: [PATCH 68/83] Fix tests for log-label changes --- test/plugins/test_lastgenre.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index 6ab163deda..a89b47bbd4 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -195,7 +195,7 @@ def test_sort_by_depth(self): { "album": ["Jazz"], }, - ("Blues, Jazz", "keep + album"), + ("Blues, Jazz", "keep + album, whitelist"), ), # 1 - force and keep whitelisted, unknown original ( @@ -211,7 +211,7 @@ def test_sort_by_depth(self): { "album": ["Jazz"], }, - ("Blues, Jazz", "keep + album"), + ("Blues, Jazz", "keep + album, whitelist"), ), # 2 - force and keep whitelisted on empty tag ( @@ -227,7 +227,7 @@ def test_sort_by_depth(self): { "album": ["Jazz"], }, - ("Jazz", "album"), + ("Jazz", "album, whitelist"), ), # 3 force and keep, artist configured ( @@ -244,7 +244,7 @@ def test_sort_by_depth(self): "album": ["Jazz"], "artist": ["Pop"], }, - ("Blues, Pop", "keep + artist"), + ("Blues, Pop", "keep + artist, whitelist"), ), # 4 - don't force, disabled whitelist ( @@ -260,7 +260,7 @@ def test_sort_by_depth(self): { "album": ["Jazz"], }, - ("Any Genre", "keep"), + ("any genre", "keep any, no-force"), ), # 5 - don't force, disabled whitelist, empty ( @@ -276,7 +276,7 @@ def test_sort_by_depth(self): { "album": ["Jazz"], }, - ("Jazz", "album"), + ("Jazz", "album, any"), ), # 6 - fallback to next stages until found ( @@ -294,7 +294,7 @@ def test_sort_by_depth(self): "album": None, "artist": ["Jazz"], }, - ("Unknown Genre, Jazz", "keep + artist"), + ("Unknown Genre, Jazz", "keep + artist, any"), ), # 7 - fallback to original when nothing found ( @@ -349,7 +349,7 @@ def test_sort_by_depth(self): { "album": ["Jazz"], }, - ("Blues\u0000Jazz", "keep + album"), + ("Blues\u0000Jazz", "keep + album, whitelist"), ), # 10 - limit a lot of results ( @@ -367,7 +367,7 @@ def test_sort_by_depth(self): { "album": ["Jazz", "Bebop", "Hardbop"], }, - ("Blues, Rock, Metal, Jazz, Bebop", "keep + album"), + ("Blues, Rock, Metal, Jazz, Bebop", "keep + album, whitelist"), ), ], ) From 4f0837c724ab343ef4d9b773bd7897fd34aea98a Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 17:21:03 +0100 Subject: [PATCH 69/83] Revert "Include lower-casing in _get_existing already" This reverts commit d5cf376a5142f974d8a6ea3b282a53a3619a8cf5. --- beetsplug/lastgenre/__init__.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 573fd6dca4..f14dcaa00b 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -303,9 +303,7 @@ def _get_existing_genres(self, obj: Union[Album, Item]) -> list[str]: else: genre_str = obj.get("genre") - return [ - g.lower() for g in genre_str.split(self.config["separator"].get()) - ] + return genre_str.split(self.config["separator"].get()) def _combine_genres( self, old: list[str], new: list[str] @@ -346,10 +344,11 @@ def _get_genre( return obj.get("genre"), "keep any, no-force" if self.config["force"]: + genres = self._get_existing_genres(obj) # Force doesn't keep any unless keep_existing is set. # Whitelist validation is handled in _resolve_genres. if self.config["keep_existing"]: - keep_genres = self._get_existing_genres(obj) + keep_genres = [g.lower() for g in genres] # Run through stages: track, album, artist, # album artist, or most popular track genre. From 6530d763191215164895e8f09de8d26acdc66a18 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 17:44:17 +0100 Subject: [PATCH 70/83] Revert "Simplify _get_existing_genres()" This reverts commit 7ff06df17cfef38c0029b5faeb193d2569220d16. This was here for a reason: Ab empty string genre should als become an empty list! --- beetsplug/lastgenre/__init__.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index f14dcaa00b..6f47e5e602 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -298,12 +298,15 @@ def fetch_track_genre(self, obj): def _get_existing_genres(self, obj: Union[Album, Item]) -> list[str]: """Return a list of genres for this Item or Album.""" + separator = self.config["separator"].get() if isinstance(obj, library.Item): - genre_str = obj.get("genre", with_album=False) + item_genre = obj.get("genre", with_album=False).split(separator) else: - genre_str = obj.get("genre") + item_genre = obj.get("genre").split(separator) - return genre_str.split(self.config["separator"].get()) + if any(item_genre): + return item_genre + return [] def _combine_genres( self, old: list[str], new: list[str] From 1219b43af4c0d41649145d1d3e4b00e3ce0a62dc Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 17:48:21 +0100 Subject: [PATCH 71/83] Fix a test: no-force always early returns --- test/plugins/test_lastgenre.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index a89b47bbd4..51aa20b816 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -276,7 +276,7 @@ def test_sort_by_depth(self): { "album": ["Jazz"], }, - ("Jazz", "album, any"), + ("Jazz", "keep any, no-force"), ), # 6 - fallback to next stages until found ( From 92e84a2b455d132527ee41eb3364f98a715f5019 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 18:06:11 +0100 Subject: [PATCH 72/83] Make sure existing genres are ALWAYS looked at first thing. Only if genres are existing and force is disabled we return early! --- beetsplug/lastgenre/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 6f47e5e602..ea9d6697af 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -340,14 +340,14 @@ def _get_genre( """ keep_genres = [] - if not self.config["force"]: + genres = self._get_existing_genres(obj) + if genres and not self.config["force"]: # Without force pre-populated tags are returned as-is. if isinstance(obj, library.Item): return obj.get("genre", with_album=False), "keep any, no-force" return obj.get("genre"), "keep any, no-force" if self.config["force"]: - genres = self._get_existing_genres(obj) # Force doesn't keep any unless keep_existing is set. # Whitelist validation is handled in _resolve_genres. if self.config["keep_existing"]: From 34b90217727b2170a78655ac74c9cd96977410fa Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 23:40:05 +0100 Subject: [PATCH 73/83] Fix test_get_genrer - configure first otherwise self.whitelist is "set up" before the test cases config is set. --- test/plugins/test_lastgenre.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index 51aa20b816..7cdfcb9e12 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -390,14 +390,14 @@ def mock_fetch_artist_genre(self, obj): lastgenre.LastGenrePlugin.fetch_album_genre = mock_fetch_album_genre lastgenre.LastGenrePlugin.fetch_artist_genre = mock_fetch_artist_genre + # Configure + config["lastgenre"] = config_values + # Initialize plugin instance and item plugin = lastgenre.LastGenrePlugin() item = _common.item() item.genre = item_genre - # Configure - config["lastgenre"] = config_values - # Run res = plugin._get_genre(item) assert res == expected_result From 261379f395da77393d00959067f78e5448bc0cb5 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Tue, 21 Jan 2025 23:54:56 +0100 Subject: [PATCH 74/83] Add test_get_genre case proving no-force keeps any because _get_existing_genres does not rely on configured separator. --- test/plugins/test_lastgenre.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index 7cdfcb9e12..e6cda923c5 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -369,6 +369,22 @@ def test_sort_by_depth(self): }, ("Blues, Rock, Metal, Jazz, Bebop", "keep + album, whitelist"), ), + # 11 - force off does not rely on configured separator + ( + { + "force": False, + "keep_existing": False, + "source": "album", + "whitelist": True, + "count": 2, + "separator": ", ", + }, + "not ; configured | separator", + { + "album": ["Jazz", "Bebop"], + }, + ("not ; configured | separator", "keep any, no-force"), + ), ], ) def test_get_genre(config_values, item_genre, mock_genres, expected_result): From 87d9f57e247f59ce1655ad9aff019cc7b9b6a49b Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Wed, 22 Jan 2025 00:09:22 +0100 Subject: [PATCH 75/83] In _get_existing_genres ensure empty string ignore --- beetsplug/lastgenre/__init__.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index ea9d6697af..ec01d75ef4 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -297,16 +297,16 @@ def fetch_track_genre(self, obj): # Main processing: _get_genre() and helpers. def _get_existing_genres(self, obj: Union[Album, Item]) -> list[str]: - """Return a list of genres for this Item or Album.""" + """Return a list of genres for this Item or Album. Empty string genres + are removed.""" separator = self.config["separator"].get() if isinstance(obj, library.Item): item_genre = obj.get("genre", with_album=False).split(separator) else: item_genre = obj.get("genre").split(separator) - if any(item_genre): - return item_genre - return [] + # Filter out empty strings + return [g for g in item_genre if g] def _combine_genres( self, old: list[str], new: list[str] From 86e1bd47a48d6159c1e661bc4eeac4aeeee0ff21 Mon Sep 17 00:00:00 2001 From: J0J0 Todos <2733783+JOJ0@users.noreply.github.com> Date: Wed, 22 Jan 2025 18:06:29 +0100 Subject: [PATCH 76/83] Single place for whitelist/any log-label suffix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Šarūnas Nejus --- beetsplug/lastgenre/__init__.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index ec01d75ef4..f50acb896c 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -404,6 +404,9 @@ def _get_genre( # Return with a combined or freshly fetched genre list. if new_genres: + suffix = "whitelist" if self.whitelist else "any" + label += f", {suffix}" + if keep_genres: label = f"keep + {label}" return self._combine_genres(keep_genres, new_genres), label From 1b05a1295d0dd6e4b5e31fe1c14982494a9637ce Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Wed, 22 Jan 2025 18:10:36 +0100 Subject: [PATCH 77/83] Revert "Further clarify lastgenre log-labels" This reverts commit 8da98e52eed4b1074b933e1953cdcc20c8daaa0f. since we applied a PR suggestions where this is done in one place. --- beetsplug/lastgenre/__init__.py | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index f50acb896c..8b04cc8938 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -338,7 +338,6 @@ def _get_genre( indicates that existing genres were combined with new last.fm genres, while "artist" means only new last.fm genres are included. """ - keep_genres = [] genres = self._get_existing_genres(obj) if genres and not self.config["force"]: @@ -360,23 +359,19 @@ def _get_genre( and "track" in self.sources and (new_genres := self.fetch_track_genre(obj)) ): - label = "track, whitelist" if self.whitelist else "track, any" + label = "track" elif "album" in self.sources and ( new_genres := self.fetch_album_genre(obj) ): - label = "album, whitelist" if self.whitelist else "album, any" + label = "album" elif "artist" in self.sources: new_genres = None if isinstance(obj, library.Item): new_genres = self.fetch_artist_genre(obj) - label = "artist, whitelist" if self.whitelist else "artist, any" + label = "artist" elif obj.albumartist != config["va_name"].as_str(): new_genres = self.fetch_album_artist_genre(obj) - label = ( - "album artist, whitelist" - if self.whitelist - else "album artist, any" - ) + label = "album artist" else: # For "Various Artists", pick the most popular track genre. item_genres = [] @@ -391,11 +386,7 @@ def _get_genre( if item_genres: most_popular, rank = plurality(item_genres) new_genres = [most_popular] - label = ( - "most popular track, whitelist" - if self.whitelist - else "most popular track, any" - ) + label = "most popular track" self._log.debug( 'Most popular track genre "{}" ({}) for VA album.', most_popular, From f695d463e2e00ed0a7859e8ab2ce06b5bd7669f4 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Wed, 22 Jan 2025 18:14:38 +0100 Subject: [PATCH 78/83] Main variables init in _get_genre() --- beetsplug/lastgenre/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 8b04cc8938..3df7a15d06 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -338,8 +338,10 @@ def _get_genre( indicates that existing genres were combined with new last.fm genres, while "artist" means only new last.fm genres are included. """ - + keep_genres = [] + label = "" genres = self._get_existing_genres(obj) + if genres and not self.config["force"]: # Without force pre-populated tags are returned as-is. if isinstance(obj, library.Item): From 5d9d8840ae4aa28584566b75dc83ae4536e90975 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Wed, 22 Jan 2025 18:20:41 +0100 Subject: [PATCH 79/83] Fix a lastgenre test --- test/plugins/test_lastgenre.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index e6cda923c5..345e6a4f9d 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -262,7 +262,7 @@ def test_sort_by_depth(self): }, ("any genre", "keep any, no-force"), ), - # 5 - don't force, disabled whitelist, empty + # 5 - don't force and empty is regular last.fm fetch; no whitelist too ( { "force": False, @@ -274,9 +274,9 @@ def test_sort_by_depth(self): }, "", { - "album": ["Jazz"], + "album": ["Jazzin"], }, - ("Jazz", "keep any, no-force"), + ("Jazzin", "album, any"), ), # 6 - fallback to next stages until found ( From 24a3394b97e36eb3abf36b9901b82293c236dceb Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Wed, 22 Jan 2025 18:28:45 +0100 Subject: [PATCH 80/83] Fix keep-none option, reword help and a tiny hint along the way: clarify that -a is implicit. --- beetsplug/lastgenre/__init__.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 3df7a15d06..9518b8f774 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -431,14 +431,14 @@ def commands(self): "--keep-existing", dest="keep_existing", action="store_true", - help="keep already present genres", + help="combine existing genres with new ones", ) lastgenre_cmd.parser.add_option( "-K", - "--keep-none", + "--no-keep-existing", dest="keep_existing", action="store_false", - help="don't keep already present genres", + help="don't combine existing genres with new ones", ) lastgenre_cmd.parser.add_option( "-s", @@ -459,7 +459,7 @@ def commands(self): "--albums", action="store_true", dest="album", - help="match albums instead of items", + help="match albums instead of items (default)", ) lastgenre_cmd.parser.set_defaults(album=True) From 2f40b315f406d4c98d08b5b6796bcc1673992751 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Wed, 22 Jan 2025 18:31:00 +0100 Subject: [PATCH 81/83] Add lastgenre --no-force, reword help --- beetsplug/lastgenre/__init__.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 9518b8f774..89556339c4 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -424,7 +424,14 @@ def commands(self): "--force", dest="force", action="store_true", - help="re-download genre when already present", + help="overwrite already present genres", + ) + lastgenre_cmd.parser.add_option( + "-F", + "--no-force", + dest="force", + action="store_false", + help="don't overwrite already present genres", ) lastgenre_cmd.parser.add_option( "-k", From 2708be257d2397988505930a99acae06c63e8e66 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Thu, 23 Jan 2025 08:31:15 +0100 Subject: [PATCH 82/83] Final nitpicks on lastgenre --help wording --- beetsplug/lastgenre/__init__.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 89556339c4..5f0955bcfe 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -424,28 +424,28 @@ def commands(self): "--force", dest="force", action="store_true", - help="overwrite already present genres", + help="modify existing genres", ) lastgenre_cmd.parser.add_option( "-F", "--no-force", dest="force", action="store_false", - help="don't overwrite already present genres", + help="don't modify existing genres", ) lastgenre_cmd.parser.add_option( "-k", "--keep-existing", dest="keep_existing", action="store_true", - help="combine existing genres with new ones", + help="combine with existing genres when modifying", ) lastgenre_cmd.parser.add_option( "-K", "--no-keep-existing", dest="keep_existing", action="store_false", - help="don't combine existing genres with new ones", + help="don't combine with existing genres when modifying", ) lastgenre_cmd.parser.add_option( "-s", From 9d4653f92f7bc39912b634f316448ad84204b4f4 Mon Sep 17 00:00:00 2001 From: J0J0 Todos Date: Thu, 23 Jan 2025 09:03:50 +0100 Subject: [PATCH 83/83] Final lastgenre docstring nitpicks and a tiny docs fix. --- beetsplug/lastgenre/__init__.py | 24 ++++++++++++------------ docs/plugins/lastgenre.rst | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 5f0955bcfe..33e40d859f 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -259,9 +259,9 @@ def _last_lookup(self, entity, method, *args): whose arguments are given in the sequence `args`. The genre lookup is cached based on the entity name and the arguments. - Before the lookup, each argument has some Unicode characters replaced - with rough ASCII equivalents in order to return better results from the - Last.fm database. + Before the lookup, each argument has the "-" Unicode character replaced + with its rough ASCII equivalents in order to return better results from + the Last.fm database. """ # Shortcut if we're missing metadata. if any(not s for s in args): @@ -322,21 +322,21 @@ def _get_genre( ) -> tuple[Union[str, None], ...]: """Get the final genre string for an Album or Item object. - `self.sources` specifies allowed genre sources (track, ablum, artist). - Together with several fallback scenarious, the following stages are run - through: + `self.sources` specifies allowed genre sources. Starting with the first + source in this tuple, the following stages run through until a genre is + found or no options are left: - track (for Items only) - album - - artist, albumartist or (for Various Artists albums) the "most - popular track genre" is used. + - artist, albumartist or "most popular track genre" (for VA-albums) - original fallback - - fallback (configured value) + - configured fallback - None A `(genre, label)` pair is returned, where `label` is a string used for - logging that describes the result. For example, "keep + artist" - indicates that existing genres were combined with new last.fm genres, - while "artist" means only new last.fm genres are included. + logging. For example, "keep + artist, whitelist" indicates that existing + genres were combined with new last.fm genres and whitelist filtering was + applied, while "artist, any" means only new last.fm genres are included + and the whitelist feature was disabled. """ keep_genres = [] label = "" diff --git a/docs/plugins/lastgenre.rst b/docs/plugins/lastgenre.rst index 54e4de7612..48a8686fe8 100644 --- a/docs/plugins/lastgenre.rst +++ b/docs/plugins/lastgenre.rst @@ -146,7 +146,7 @@ Add new last.fm genres when **empty**. Any present tags stay **untouched**. **Combine** genres in present tags with new ones (be aware of that with an enabled ``whitelist`` setting, of course some genres might get cleaned up. To -make sure any exisitng genres remain, set ``whitelist: no``). +make sure any existing genres remain, set ``whitelist: no``). .. code-block:: yaml