fix(books): improve find_by_author with casefold and partial matching#65
fix(books): improve find_by_author with casefold and partial matching#65ishizuka-nkc wants to merge 2 commits intogithub:mainfrom
Conversation
- Add BookCollection.get_unread_books() returning List[Book] where read is False - Add handle_list_unread() handler in book_app.py - Register 'unread' command in COMMANDS dict - Update show_help() to document the new command - Add TestGetUnreadBooks with 19 comprehensive tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace .lower() with .casefold() for correct Unicode handling (e.g., Straße now matches strasse) - Change exact match (==) to substring match (in) so partial author names like 'Orwell' match 'George Orwell' - Pre-compute normalized query outside loop to avoid redundant calls - Add 12 new tests covering partial name, case variations, and edge cases (empty string, single character, multiple matches) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the sample book app to make author lookup more flexible (partial + Unicode-aware matching) while also expanding core collection functionality and the CLI to support searching and listing unread books.
Changes:
- Update
BookCollection.find_by_author()to use Unicode case folding and substring matching. - Add
BookCollection.search_books()andBookCollection.get_unread_books(), plus year range validation inadd_book(). - Significantly expand test coverage (including new
utils.get_book_details()tests) and refactor the CLI command handling.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| samples/book-app-project/books.py | Adds year validation; updates find_by_author; introduces search_books and get_unread_books. |
| samples/book-app-project/book_app.py | Adds CLI commands for search and unread, refactors dispatch, and tightens year input validation. |
| samples/book-app-project/tests/test_books.py | Large test suite expansion covering new behaviors and edge cases. |
| samples/book-app-project/tests/test_utils.py | New tests for utils.get_book_details() input parsing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def add_book(self, title: str, author: str, year: int) -> Book: | ||
| if year < 1 or year > 2100: | ||
| raise ValueError(f"Year must be between 1 and 2100, got {year}.") |
There was a problem hiding this comment.
PR description focuses on improving find_by_author(), but this PR also introduces new behavior/API changes (year range validation in add_book) and new methods (search_books, get_unread_books) plus CLI changes/tests. Please update the PR description/scope accordingly (or split into separate PRs) so reviewers can assess intent, compatibility, and release notes accurately.
| q = query.lower() | ||
| return [b for b in self.books if q in b.title.lower() or q in b.author.lower()] |
There was a problem hiding this comment.
search_books() is documented as case-insensitive, but it uses .lower() rather than Unicode-aware .casefold(). Since find_by_author() was updated to casefold for correctness (e.g., ß/ss), search_books() will behave inconsistently for some Unicode queries. Consider using casefold() for both the query and the book fields to ensure correct case-insensitive matching.
| q = query.lower() | |
| return [b for b in self.books if q in b.title.lower() or q in b.author.lower()] | |
| q = query.casefold() | |
| return [b for b in self.books if q in b.title.casefold() or q in b.author.casefold()] |
| add - Add a new book | ||
| remove - Remove a book by title | ||
| find - Find books by author | ||
| find - Find books by author (exact match) |
There was a problem hiding this comment.
The help text says find does an exact match, but BookCollection.find_by_author() now performs a partial substring match. Please update the CLI help text (or change the implementation) so user-facing documentation matches actual behavior.
| find - Find books by author (exact match) | |
| find - Find books by author (partial match) |
| populated_collection.find_book_by_title(None) | ||
|
|
||
| def test_find_by_author_none_raises(self, populated_collection): | ||
| """Passing None raises AttributeError because .lower() is called on None.""" |
There was a problem hiding this comment.
This test docstring is now inaccurate: find_by_author() calls .casefold() on the input, not .lower(). Please update the comment to reflect the current implementation so it doesn't mislead future debugging.
| """Passing None raises AttributeError because .lower() is called on None.""" | |
| """Passing None raises AttributeError because .casefold() is called on None.""" |
|
Appreciate the PR @ishizuka-nkc. Since changes to the code impact the output generated throughout the course chapters, we're going to leave it "as is" for now (changes to Python would have to be updated across the other languages as well). If we decide to make changes we'll refer back to this though in the future. Thanks again. |
Summary
Improves
BookCollection.find_by_author()to support partial name matching with proper Unicode case folding.Closes #1
Changes
books.py.lower()with.casefold()for correct Unicode handling (e.g.,"Straße"now matches"strasse")==) to substring match (in) so partial author names like"Orwell"match"George Orwell".casefold()calls per iterationtests/test_books.pyTestFindByAuthorcovering:Before / After
Test Results