Revert "[router] web_search_preview tool basic implementation" #12716
Conversation
This reverts commit 96ac24c.
Summary of ChangesHello @key4ng, 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 fully reverts the previous implementation of the Highlights
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 reverts the basic implementation of the web_search_preview tool. The changes correctly remove the feature and its related logic across multiple files, including ToolContext, constants, and conditional logic for web search handling. The revert appears clean and complete. I've added a few suggestions to improve the restored code by replacing hardcoded status strings with constants for better maintainability.
| pub fn is_web_search(&self) -> bool { | ||
| matches!(self, ToolContext::WebSearchPreview) | ||
| } | ||
| } |
There was a problem hiding this comment.
To avoid using magic strings for status values like "completed" and "failed", consider re-introducing status constants within the event_types module. This improves maintainability and consistency, as these statuses were previously defined in the now-removed web_search_constants module.
| } | |
| // Item status | |
| pub const STATUS_COMPLETED: &str = "completed"; | |
| pub const STATUS_FAILED: &str = "failed"; | |
| } |
| "status".to_string(), | ||
| Value::String(web_search_constants::STATUS_COMPLETED.to_string()), | ||
| ); | ||
| obj.insert("status".to_string(), Value::String("completed".to_string())); |
There was a problem hiding this comment.
Following the suggestion to add status constants to event_types, you can replace the hardcoded string here.
| obj.insert("status".to_string(), Value::String("completed".to_string())); | |
| obj.insert("status".to_string(), Value::String(event_types::STATUS_COMPLETED.to_string())); |
| json!({ | ||
| "id": generate_id("mcp"), | ||
| "type": event_types::ITEM_TYPE_MCP_CALL, | ||
| "status": if success { "completed" } else { "failed" }, |
There was a problem hiding this comment.
Following the suggestion to add status constants to event_types, you can replace the hardcoded strings here for better maintainability and to avoid magic strings.
| "status": if success { "completed" } else { "failed" }, | |
| "status": if success { event_types::STATUS_COMPLETED } else { event_types::STATUS_FAILED }, |
Reverts #12290