lastgenre: Use albumartists field to improve last.fm results

[JOJ0]

Often last.fm does not find artist genres for delimiter-separated artist names (eg. "Artist One, Artist Two") or where multiple artists are combined with "concatenation words" like "and" , "+", "featuring" and so on.

This fix gathers each artist's last.fm genre separately by using Beets' mutli-valued albumartists field to improve the likeliness of finding genres in the artist genre fetching stage.

Refactoring was done along the existing genre fetching helper functions (fetch_album_genre, fetch_track_genre, ...):

• last.fm can be asked for genre for these combinations of metadata:

  • albumartist/album
  • artist/track
  • artist

• Instead of passing Album or Item objects directly to these helpers., generalize them and pass the (string) metadata directly.

• Passing "what's to fetch" in the callers instead of hiding it in the methods also helps readability in _get_genre()

• And reduces the requirement at hand for another additional method (or adaptation) to support "multi-albumartist genre fetching"

• Documentation

• Changelog

• Tests

[github-actions]

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

[codecov]

Codecov Report

❌ Patch coverage is 34.78261% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.22%. Comparing base (1b701c8) to head (28dc78b).
⚠️ Report is 52 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
beetsplug/lastgenre/__init__.py	34.78%	14 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5981      +/-   ##
==========================================
- Coverage   68.25%   68.22%   -0.03%     
==========================================
  Files         138      138              
  Lines       18785    18791       +6     
  Branches     3164     3167       +3     
==========================================
- Hits        12821    12820       -1     
- Misses       5291     5298       +7     
  Partials      673      673              

Files with missing lines	Coverage Δ
beetsplug/lastgenre/__init__.py	69.81% <34.78%> (-1.93%)	⬇️

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sourcery-ai]

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `beetsplug/lastgenre/__init__.py:431-437` </location>
<code_context>
-                new_genres = self.fetch_album_artist_genre(obj)
+                new_genres = self.fetch_artist_genre(obj.albumartist)
                 stage_label = "album artist"
+                if not new_genres:
+                    self._tunelog(
+                        'No album artist genre found for "{}", '
+                        "trying multi-valued field...",
+                        obj.albumartist,
+                    )
+                    for albumartist in obj.albumartists:
+                        self._tunelog(
+                            'Fetching artist genre for "{}"', albumartist
</code_context>

<issue_to_address>
**suggestion:** Potential duplication of genres when aggregating from multiple album artists.

Deduplicate the genre list before returning or further processing to avoid repeated entries.

```suggestion
                    for albumartist in obj.albumartists:
                        self._tunelog(
                            'Fetching artist genre for "{}"', albumartist
                        )
                        new_genres += self.fetch_artist_genre(albumartist)
                    # Deduplicate genres while preserving order
                    if new_genres:
                        new_genres = list(dict.fromkeys(new_genres))
                        stage_label = "multi-valued album artist"
```
</issue_to_address>

### Comment 2
<location> `beetsplug/lastgenre/__init__.py:441` </location>
<code_context>
             else:
                 # For "Various Artists", pick the most popular track genre.
                 item_genres = []
+                assert isinstance(obj, Album)  # Type narrowing for mypy
                 for item in obj.items():
                     item_genre = None
</code_context>

<issue_to_address>
**suggestion:** Use runtime type checking instead of assert for production code.

Consider replacing the assert with an explicit isinstance check and appropriate error handling, since asserts can be disabled in production environments.

```suggestion
                if not isinstance(obj, Album):
                    raise TypeError(f"Expected obj to be an Album, got {type(obj).__name__}")
```
</issue_to_address>

### Comment 3
<location> `test/plugins/test_lastgenre.py:555` </location>
<code_context>
         return mock_genres["album"]

-    def mock_fetch_artist_genre(self, obj):
+    def mock_fetch_artist_genre(self, artist):
         return mock_genres["artist"]

</code_context>

<issue_to_address>
**suggestion (testing):** No test coverage for multi-valued albumartists genre fetching.

Please add a test that creates an Album with multiple albumartists and verifies that genres are fetched for each and aggregated correctly.

Suggested implementation:

```python
    def mock_fetch_artist_genre(self, artist):

```

```python
    def test_album_with_multiple_albumartists_genre_aggregation(self):
        # Setup mock genres for two artists
        artist1 = "Artist One"
        artist2 = "Artist Two"
        genre_artist1 = ["rock", "indie"]
        genre_artist2 = ["pop", "electronic"]

        # Patch the genre fetcher to return different genres for each artist
        def genre_fetcher(artist):
            if artist == artist1:
                return genre_artist1
            elif artist == artist2:
                return genre_artist2
            return []

        # Assume Album and LastGenrePlugin are available in the test context
        album = Album(albumartist=[artist1, artist2], albumtitle="Test Album")
        plugin = LastGenrePlugin()
        plugin.fetch_artist_genre = genre_fetcher

        # Fetch genres for the album
        genres = []
        for artist in album.albumartist:
            genres.extend(plugin.fetch_artist_genre(artist))
        # Aggregate and deduplicate genres
        aggregated_genres = sorted(set(genres))

        # Assert that all genres from both artists are present
        expected_genres = sorted(set(genre_artist1 + genre_artist2))
        assert aggregated_genres == expected_genres, f"Expected {expected_genres}, got {aggregated_genres}"

```
</issue_to_address>

### Comment 4
<location> `test/plugins/test_lastgenre.py:552-553` </location>
<code_context>
         return mock_genres["track"]

-    def mock_fetch_album_genre(self, obj):
+    def mock_fetch_album_genre(self, albumartist, albumtitle):
         return mock_genres["album"]

</code_context>

<issue_to_address>
**suggestion (testing):** No test for fallback when album artist genre is missing.

Please add a test that simulates an empty result from the primary albumartist genre fetch and confirms the fallback logic is executed correctly.

```suggestion
    def mock_fetch_album_genre(self, albumartist, albumtitle):
        return mock_genres["album"]

def test_album_genre_fallback(monkeypatch):
    """Test fallback logic when albumartist genre is missing."""

    class DummyGenreFetcher:
        def fetch_album_genre(self, albumartist, albumtitle):
            # Simulate missing genre for albumartist
            return None

        def fetch_albumtitle_genre(self, albumtitle):
            # Simulate fallback genre for albumtitle
            return "FallbackGenre"

    fetcher = DummyGenreFetcher()

    # Patch the primary fetch to return None, and fallback to albumtitle genre
    monkeypatch.setattr(fetcher, "fetch_album_genre", lambda albumartist, albumtitle: None)
    monkeypatch.setattr(fetcher, "fetch_albumtitle_genre", lambda albumtitle: "FallbackGenre")

    # Simulate the logic that tries albumartist first, then falls back to albumtitle
    genre = fetcher.fetch_album_genre("MissingArtist", "TestAlbum")
    if genre is None:
        genre = fetcher.fetch_albumtitle_genre("TestAlbum")

    assert genre == "FallbackGenre"
```
</issue_to_address>

### Comment 5
<location> `test/plugins/test_lastgenre.py:549` </location>
<code_context>
     """Test _get_genre with various configurations."""

-    def mock_fetch_track_genre(self, obj=None):
+    def mock_fetch_track_genre(self, trackartist, tracktitle):
         return mock_genres["track"]

</code_context>

<issue_to_address>
**suggestion (testing):** Tests do not cover delimiter-separated or concatenated artist names.

Please add tests with delimiter-separated or concatenated artist names in the albumartists field to ensure genres are correctly fetched for each artist.
</issue_to_address>

### Comment 6
<location> `docs/changelog.rst:50` </location>
<code_context>
   endpoints. Previously, due to single-quotes (ie. string literal) in the SQL
   query, the query eg. `GET /item/values/albumartist` would return the literal
   "albumartist" instead of a list of unique album artists.
+- :doc:`plugins/lastgenre`: Fix the issue were last.fm does not give a result in
+  the artist genre stage because multi-artists "concatenation" words (like
+  "feat." "+", or "&" prevent exact matches. Using the albumartists list field
</code_context>

<issue_to_address>
**issue (typo):** Typo: 'were' should be 'where'.

Please change 'were' to 'where' in the documentation for correct grammar.

```suggestion
- :doc:`plugins/lastgenre`: Fix the issue where last.fm does not give a result in
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[JOJ0]

Hi @henry-oberholtzer finally gotten around to add type hints to the _fetch_... methods. I realized that the plugin at this point was not far from full type hint coverage, so I finally added ALL of them even though some of them are not related to this PR.

Slightly more hassle for your re-review but I'm sure you'll manage. It's not tooo much actually and I finally would like to get this task on my bucket list done. Thanks in advance!

[JOJ0]

... so I finally added ALL of them even though some of them are not related to this PR.

I reconsidered and split out the remaining types to a new PR #6239 . Makes both of them easier to read and review. Thanks in advance for a final look on this one as a first step.

[henry-oberholtzer]

No problem! I'm pro more type hints everywhere so happy to review that one too. I should be able to take a look at it this weekend.

[henry-oberholtzer]

Looks great!!