Skip to content

Multi fields #38

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
steven-murray merged 2 commits into from
Feb 3, 2025
Merged

Multi fields #38

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96fa9cf
feat: split multi-field items by semi-colon
steven-murray Feb 3, 2025
ebd119c
feat: split multi-field items by semi-colon with tests
steven-murray Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions beetsplug/summarize.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,16 +118,28 @@ def set_str_converter(stat, stat_type):
stat["str_converter"] = "len"


def group_by(category, items):
"""Group a list of items by a category."""
def group_by(category: str, items):
"""Group a list of items by a category.

If the category is one that supports multiple values, split them by ";" and add
the item to each of the groups.
"""
Comment on lines +121 to +126
Copy link

@sourcery-ai sourcery-ai bot Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Consider handling edge cases where 'cat' is None or an empty string

The current implementation might create groups with empty keys or raise exceptions for None values. Consider adding validation or explicit handling for these cases.

Suggested change
def group_by(category: str, items):
"""Group a list of items by a category.
If the category is one that supports multiple values, split them by ";" and add
the item to each of the groups.
"""
def group_by(category: str | None, items: list) -> dict:
"""Group a list of items by a category.
If the category is one that supports multiple values, split them by ";" and add
the item to each of the groups.
Args:
category: The category to group by. If None or empty string, returns empty dict.
items: List of items to group.
Returns:
A dictionary mapping category values to lists of items.
"""
if not category: # Handles None and empty string
return {}

multifield_categories = ["albumartist", "artist", "genre"]

out = {}
for item in items:
cat = getattr(item, category)
if category in multifield_categories:
cats = [c.strip() for c in cat.split(";")]
else:
cats = [cat]

for cat in cats:

if cat not in out:
out[cat] = []
if cat not in out:
out[cat] = []

out[cat].append(item)
out[cat].append(item)

return out

Expand Down
21 changes: 21 additions & 0 deletions tests/test_summarize.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,17 @@ def lib():
lib.add(MockItem("song1", 2000, "artist1", "album1", 128, "lyrics1"))
lib.add(MockItem("song2", 2001, "artist2", "album2", 256, "lyrics2"))
lib.add(MockItem("song3", 2002, "artist3", "album3", 512, "lyrics3"))
lib.add(
MockItem(
"song4",
2003,
"artist3; artist4",
"album4",
700,
"so many lyrics in this song",
)
Comment on lines +37 to +45
Copy link

@sourcery-ai sourcery-ai bot Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Suggest adding more test cases for multi-field handling with edge cases.

It would be beneficial to add tests for edge cases like empty strings, strings with only delimiters, and combinations of valid and invalid multi-field values. For example, consider cases like 'artist1;;artist2', ';artist1;artist2;', ' ', and '' to ensure the function handles these scenarios correctly.

Suggested implementation:

    lib.add(MockItem("song1", 2000, "artist1", "album1", 128, "lyrics1"))
    lib.add(MockItem("song2", 2001, "artist2", "album2", 256, "lyrics2"))
    lib.add(MockItem("song3", 2002, "artist3", "album3", 512, "lyrics3"))
    lib.add(
        MockItem(
            "song4",
            2003,
            "artist3; artist4",
            "album4",
            700,
            "so many lyrics in this song",
        )
    )
    # Edge cases for multi-field handling
    lib.add(
        MockItem(
            "song5",
            2004,
            "artist1;;artist2",  # Double delimiter
            "album5",
            800,
            "lyrics5",
        )
    )
    lib.add(
        MockItem(
            "song6",
            2005,
            ";artist1;artist2;",  # Leading/trailing delimiters
            "album6",
            900,
            "lyrics6",
        )
    )
    lib.add(
        MockItem(
            "song7",
            2006,
            " ",  # Just whitespace
            "album7",
            1000,
            "lyrics7",
        )
    )
    lib.add(
        MockItem(
            "song8",
            2007,
            "",  # Empty string
            "album8",
            1100,
            "lyrics8",
        )
    )
    lib.add(
        MockItem(
            "song9",
            2008,
            " artist1 ; artist2 ",  # Whitespace around delimiters
            "album9",
            1200,
            "lyrics9",
        )
    )

    return lib

You'll need to:

  1. Add corresponding assertions in the test functions to verify these edge cases are handled correctly
  2. Ensure the library's parsing logic properly handles these edge cases (empty strings, multiple delimiters, whitespace)
  3. Update any relevant documentation to describe the expected behavior for these edge cases

)

Copy link

@sourcery-ai sourcery-ai bot Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Missing tests for other multifield_categories.

The description mentions that multiple fields are now supported. The tests only cover the artist field. Add tests for albumartist and genre as well to ensure complete coverage of the new functionality.

Suggested implementation:

    lib.add(
        MockItem(
            "song4",
            2003,
            "artist3; artist4",
            "album4",
            700,
            "so many lyrics in this song",
        )
    )

    # Add items with multiple albumartists
    lib.add(
        MockItem(
            "song5",
            2004,
            "artist5",
            "album5",
            800,
            "lyrics5",
            albumartist="albumartist1; albumartist2"
        )
    )

    # Add items with multiple genres
    lib.add(
        MockItem(
            "song6",
            2005,
            "artist6",
            "album6",
            900,
            "lyrics6",
            genre="rock; pop"
        )
    )

    return lib


    assert "2000 |" in txt
def test_show_summary_artist(lib):
    stats = "count"
    txt = sm.show_summary(lib, "query", category="artist", stats=stats, reverse=False)

    assert "artist1 | 1" in txt
    assert "artist2 | 1" in txt
    assert "artist3 | 2" in txt  # in the multi-field
    assert "artist4 | 1" in txt  # in the multi-field

def test_show_summary_albumartist(lib):
    stats = "count"
    txt = sm.show_summary(lib, "query", category="albumartist", stats=stats, reverse=False)

    assert "albumartist1 | 1" in txt  # in the multi-field
    assert "albumartist2 | 1" in txt  # in the multi-field

def test_show_summary_genre(lib):
    stats = "count"
    txt = sm.show_summary(lib, "query", category="genre", stats=stats, reverse=False)

    assert "rock | 1" in txt  # in the multi-field
    assert "pop | 1" in txt   # in the multi-field

Note: The MockItem class constructor might need to be updated to accept albumartist and genre parameters if they're not already supported. You may need to add these fields to the MockItem class definition if they don't exist.

return lib


Expand Down Expand Up @@ -78,3 +89,13 @@ def test_show_summary(lib):
assert "2000 |" in txt
assert "2001 |" in txt
assert "2002 |" in txt


def test_show_summary_artist(lib):
stats = "count"
txt = sm.show_summary(lib, "query", category="artist", stats=stats, reverse=False)

assert "artist1 | 1" in txt
assert "artist2 | 1" in txt
assert "artist3 | 2" in txt # in the multi-field
assert "artist4 | 1" in txt # in the multi-field
Comment on lines +100 to +101
Copy link

@sourcery-ai sourcery-ai bot Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Consider using more precise assertions.

Instead of checking if a substring exists in the output, it's better to parse the output and assert on the specific values. This makes the test less brittle and provides more informative error messages when it fails. For example, you could split the output by lines and then by '|' to check the counts for each artist.

Loading