Conversation
Add to the resolve_partial_id function to look at the title as well (instead of just ID). Updated the code based on comment (in the event title is blank) for security reason.
Summary of ChangesHello @Tikkel77, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request enhances the partial ID resolution logic to also search by title, which is a great usability improvement. My review focuses on two areas: first, a minor issue where a code comment and a related unit test have become outdated due to a change in an ID length threshold. Second, I've proposed a change to the search logic to combine results from both ID and title searches. This will ensure more comprehensive and intuitive results, especially in cases where a search term could ambiguously match either an ID prefix or a title.
| # Skip resolution for IDs that look complete (20+ chars) | ||
| if len(partial_id) >= 20: | ||
| if len(partial_id) >= 50: |
There was a problem hiding this comment.
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_resolution in tests/unit/cli/test_resolve.py to fail, as it asserts behavior based on the old 20-character threshold. Please update the test accordingly.
| # Skip resolution for IDs that look complete (20+ chars) | |
| if len(partial_id) >= 20: | |
| if len(partial_id) >= 50: | |
| # Skip resolution for IDs that look complete (50+ chars) | |
| if len(partial_id) >= 50: |
| 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())] |
There was a problem hiding this comment.
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.
| 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())] | |
| matches = [item for item in items if item.id.lower().startswith(partial_id.lower()) or (item.title or "").lower().startswith(partial_id.lower())] |
- Add title prefix matching as fallback when ID prefix match yields no results - Improve error message to indicate both ID and title are searched - Fix ambiguous input message wording This resolves teng-lin#115 and teng-lin#113
Add to the resolve_partial_id function to look at the title as well (instead of just ID).
Updated the code based on comment (in the event title is blank) for security reason.