- Notifications
You must be signed in to change notification settings - Fork 2
feat(strings, anagrams): group anagrams #98
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.
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
Conversation
| Caution Review failedThe pull request is closed. WalkthroughAdds a new "Group Anagrams" feature: README and DIRECTORY entry, two implementations (sorted-key and frequency-count) and a unittest suite; also adds the parameterized test dependency to pyproject.toml. Changes
Sequence Diagram(s)sequenceDiagram participant Caller as Test / User participant Module as group_anagrams module participant Map as Group Map Note over Caller,Module: Input list of strings Caller->>Module: call group_anagrams(strs) alt frequency-count key (new) Module->>Module: compute 26-element freq tuple per string else sorted-key (naive) Module->>Module: sort lowercased string to build key end Module->>Map: append string to group keyed by key Module->>Caller: return list(Map.values()) as List[List[str]] Note over Caller,Module: Order of groups / order within groups not guaranteed Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pystrings/anagram/group_anagrams/test_group_anagrams.py (2)
2-2: Consider testing both implementations.The test suite only imports and tests
group_anagrams, but the module exports bothgroup_anagramsandgroup_anagrams_naive. Consider adding test coverage for the naive implementation as well.You could parameterize the tests to cover both implementations:
import unittest from parameterized import parameterized from . import group_anagrams, group_anagrams_naive class GroupAnagramsTestCase(unittest.TestCase): @parameterized.expand([ (group_anagrams,), (group_anagrams_naive,), ]) def test_1(self, func): strs = ["eat", "beat", "neat", "tea"] expected = [["eat", "tea"], ["beat"], ["neat"]] actual = func(strs) self.assertEqual(expected, actual) # ... repeat for other tests
6-46: Tests may be fragile due to exact ordering assumption.The tests use
assertEqualwith exact list ordering (e.g.,[["eat", "tea"], ["beat"], ["neat"]]). While Python 3.7+ dictionaries maintain insertion order, this creates fragile tests that depend on implementation details rather than semantic correctness.The README notes "the order in which the output is displayed doesn't matter," but these tests enforce a specific order.
Consider comparing sets instead:
def test_1(self): strs = ["eat", "beat", "neat", "tea"] expected = [["eat", "tea"], ["beat"], ["neat"]] actual = group_anagrams(strs) # Convert to set of frozensets for order-independent comparison self.assertEqual( {frozenset(group) for group in expected}, {frozenset(group) for group in actual} )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pystrings/anagram/group_anagrams/images/group_anagrams_example_one.pngis excluded by!**/*.pngpystrings/anagram/group_anagrams/images/group_anagrams_example_two.pngis excluded by!**/*.png
📒 Files selected for processing (4)
DIRECTORY.md(1 hunks)pystrings/anagram/group_anagrams/README.md(1 hunks)pystrings/anagram/group_anagrams/__init__.py(1 hunks)pystrings/anagram/group_anagrams/test_group_anagrams.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pystrings/anagram/group_anagrams/test_group_anagrams.py (1)
pystrings/anagram/group_anagrams/__init__.py (1)
group_anagrams(24-69)
🪛 markdownlint-cli2 (0.18.1)
DIRECTORY.md
679-679: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
680-680: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
Describe your change:
This adds the group anagram code challenge to group anagrams together from a list of provided strings.
Checklist:
Fixes: #{$ISSUE_NO}.Summary by CodeRabbit
New Features
Documentation
Tests