feat: support searching by title in partial ID resolver#158
feat: support searching by title in partial ID resolver#158Jah-yee wants to merge 1 commit intoteng-lin:mainfrom
Conversation
- 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
Summary of ChangesHello, 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 significantly enhances the partial ID resolution logic within the CLI by introducing the ability to search for items by their title as a fallback when a direct ID match is not found. This improves user experience by making the CLI more flexible and forgiving, allowing users to locate notebooks, sources, or artifacts using more intuitive title-based queries in addition to partial IDs. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a useful feature to allow searching for entities by title prefix as a fallback to matching by ID. The implementation is straightforward. I've identified a minor performance improvement opportunity by avoiding repeated string operations within a loop. More importantly, the new functionality is not covered by unit tests, which is crucial for ensuring its correctness and preventing future regressions. Please add tests to cover the new title-matching logic.
Note: Security Review is unavailable for this PR.
|
|
||
| Allows users to type partial IDs like 'abc' instead of full UUIDs. | ||
| Matches are case-insensitive prefix matches. | ||
| Also supports searching by title (prefix match, case-insensitive). |
There was a problem hiding this comment.
This is a great feature enhancement. However, the new functionality of searching by title is not covered by unit tests. Please add tests to tests/unit/cli/test_resolve.py to cover various scenarios, such as:
- Matching by a unique title prefix.
- Prioritizing ID matches over title matches when both could match.
- Handling ambiguous title matches.
- Ensuring case-insensitivity for title search.
- Verifying behavior with items that have no title.
References
- When a function's signature is updated to return new values, update tests to assert the state of these new returns, including for error handling and early-return paths.
| matches = [item for item in items if item.id.lower().startswith(partial_id.lower())] | ||
|
|
||
| # If no ID matches, try title prefix match | ||
| if len(matches) == 0: | ||
| matches = [item for item in items if item.title and item.title.lower().startswith(partial_id.lower())] |
There was a problem hiding this comment.
This implementation calls partial_id.lower() inside the list comprehensions, which is inefficient as it's re-evaluated for every item. It's better to compute the lowercase version of partial_id once before the loops. Also, using if not matches: is more idiomatic than if len(matches) == 0:.
| matches = [item for item in items if item.id.lower().startswith(partial_id.lower())] | |
| # If no ID matches, try title prefix match | |
| if len(matches) == 0: | |
| matches = [item for item in items if item.title and item.title.lower().startswith(partial_id.lower())] | |
| lower_partial_id = partial_id.lower() | |
| # First try ID prefix match | |
| matches = [item for item in items if item.id.lower().startswith(lower_partial_id)] | |
| # If no ID matches, try title prefix match | |
| if not matches: | |
| matches = [item for item in items if item.title and item.title.lower().startswith(lower_partial_id)] |
1 participant
Summary
This PR adds support for searching by title in the partial ID resolver. When a user provides a partial ID that doesn't match any notebook/source/artifact ID, the resolver now falls back to searching by title prefix match.
Changes
Testing
Related Issues
Contributed as part of Spark Lab AI Agent exploration