-
Notifications
You must be signed in to change notification settings - Fork 430
Update helpers.py to search by title #115
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -275,11 +275,15 @@ async def _resolve_partial_id( | |||||||||||||
| partial_id = validate_id(partial_id, entity_name) | ||||||||||||||
|
|
||||||||||||||
| # Skip resolution for IDs that look complete (20+ chars) | ||||||||||||||
| if len(partial_id) >= 20: | ||||||||||||||
| if len(partial_id) >= 50: | ||||||||||||||
| return partial_id | ||||||||||||||
|
|
||||||||||||||
| items = await list_fn() | ||||||||||||||
| matches = [item for item in items if item.id.lower().startswith(partial_id.lower())] | ||||||||||||||
|
|
||||||||||||||
| # Do a match based on title if there is no match on UUID: | ||||||||||||||
| if len(matches) == 0: | ||||||||||||||
| matches = [item for item in items if (item.title or "").lower().startswith(partial_id.lower())] | ||||||||||||||
|
Comment on lines
282
to
+286
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current logic prioritizes ID matches and only searches by title if no ID matches are found. This can lead to confusing behavior if a user's search term matches an ID prefix but they intended to find an item by its title. For example, searching for 'action' might match an item with ID 'action-item-id' and ignore another item with the title 'action plan'. To provide more intuitive results, it would be better to search by both ID and title, and then present all unique matches to the user. This way, if there are multiple possibilities, the user is notified of the ambiguity.
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| if len(matches) == 1: | ||||||||||||||
| if matches[0].id != partial_id: | ||||||||||||||
|
|
||||||||||||||
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.
The comment on line 277 is now out of sync with the code on line 278. Please update the comment to reflect the new threshold of 50 characters.
Additionally, this change will likely cause the unit test
TestResolveNotebookId.test_long_id_skips_resolutionintests/unit/cli/test_resolve.pyto fail, as it asserts behavior based on the old 20-character threshold. Please update the test accordingly.