Remove legacy search indicators logic and config#166
Remove legacy search indicators logic and config#166keyurva merged 5 commits intodatacommonsorg:mainfrom
Conversation
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 streamlines the search indicator mechanism by deprecating and removing legacy configurations and client-side logic. The change ensures that all search requests for indicators are now uniformly handled by 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 effectively removes the legacy search indicators configuration and logic, routing all search requests through the fetch_indicators method from the datacommons-client library. The changes are comprehensive, touching configuration files, client logic, data models, and tests. This refactoring simplifies the codebase by removing the use_search_indicators flag and related code paths. The tests have been updated to reflect these changes, ensuring the new consolidated logic is covered. A minor point of improvement is identified regarding some leftover logic from the previous implementation, which is addressed in the review comments.
clincoln8
left a comment
There was a problem hiding this comment.
Functionally lgtm, but I think we can do some more cleanup to improve clarity and streamline the logic a bit. That can be done in a followup.
| @@ -616,25 +485,24 @@ async def fetch_indicators( | |||
| } | |||
|
|
|||
| async def _search_vector( | |||
There was a problem hiding this comment.
Can we refactor some of these methods and/or rename them? I think the current flow is
services.search_indicators -> services._search_vector -> client.fetch_indicators -> client._search_vector -> client._call_fetch_indicators
which is hard to follow and not clear what each step is actually doing or why it's grouped the way it is.
This could be a follow up as well.
There was a problem hiding this comment.
Agree. Will do this in a follow up. And will look into the async question in the other comment as well.
| ) | ||
| logger.info("Calling client library fetch_indicators for: '%s'", query) | ||
| # Run the synchronous client method in a thread | ||
| search_results = await asyncio.to_thread( |
There was a problem hiding this comment.
does this need to be in a thread and does this overall (clients._search_vector) method need to be async?
use_search_indicatorsand other related settings.fetch_indicatorsmethod in the datacommons-client library.