Multi fields #38
Merged
Multi fields #38
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request introduces multi-field support for the Class diagram showing updated group_by functionclassDiagram
class group_by {
+multifield_categories: list
+group_by(category: str, items: list) dict
}
note for group_by "New multifield support for albumartist, artist, genre"
Flow diagram for multi-field grouping processgraph TD
A[Start] --> B[Get item's category value]
B --> C{Is category multi-field?}
C -->|Yes| D[Split value by ';']
C -->|No| E[Use value as is]
D --> F[Strip whitespace]
E --> G[Add item to group]
F --> G
G --> H{More items?}
H -->|Yes| B
H -->|No| I[End]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @steven-murray - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+121
to
+126
| 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. | ||
| """ |
There was a problem hiding this comment.
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 {} |
Comment on lines
+37
to
+45
| lib.add( | ||
| MockItem( | ||
| "song4", | ||
| 2003, | ||
| "artist3; artist4", | ||
| "album4", | ||
| 700, | ||
| "so many lyrics in this song", | ||
| ) |
There was a problem hiding this comment.
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 libYou'll need to:
- Add corresponding assertions in the test functions to verify these edge cases are handled correctly
- Ensure the library's parsing logic properly handles these edge cases (empty strings, multiple delimiters, whitespace)
- Update any relevant documentation to describe the expected behavior for these edge cases
Comment on lines
+100
to
+101
| assert "artist3 | 2" in txt # in the multi-field | ||
| assert "artist4 | 1" in txt # in the multi-field |
There was a problem hiding this comment.
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.
| "so many lyrics in this song", | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
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 txtdef 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-fieldNote: 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #37
Allows some fields to be multi-field capable, separated by ';'. In this case, the track gets added to each group.
Summary by Sourcery
Bug Fixes: