Add current time functionality to root agent #3472
Conversation
Implemented a function to get the current time in a specified city and integrated it into the root agent.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @saeedtamboli01, 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 introduces a new 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
|
|
Response from ADK Triaging Agent Hello @saeedtamboli01, thank you for creating this PR! Before we can proceed with the review, could you please address the following points from our contribution guidelines:
This information is essential for reviewers to understand and verify your contribution efficiently. You can find more details in our contribution guide. Thank you! |
![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 introduces a new agent to provide the current time. However, the core function get_current_time has a significant flaw: it doesn't use the city parameter and always returns the server's local time, which contradicts its purpose. I've provided a review comment with a suggested fix that implements a more accurate, albeit still example-level, version using a city-to-timezone mapping and Python's zoneinfo module. This change will make the function behave as expected for a few sample cities.
| def get_current_time(city: str) -> dict: | ||
| """ | ||
| Returns the current time in the specified city. | ||
| """ | ||
| # For example purpose only. Ideally, integrate with a world time API. | ||
| time_now = datetime.datetime.now().strftime('%I:%M %p') | ||
| return {"status": "success", "city": city, "time": time_now} |
There was a problem hiding this comment.
The get_current_time function currently ignores the city parameter and always returns the local time of the server. This is misleading as the function signature and docstring imply it will return the time for the specified city. Even for an example, it's better to have a more realistic implementation that uses the input parameter.
I suggest a mock implementation using a small dictionary to map cities to IANA timezones and Python's built-in zoneinfo module (available in Python 3.9+) to get the correct time. This makes the function work as advertised for a few example cities and provides a better foundation for a real implementation. You'll also need to add from zoneinfo import ZoneInfo, ZoneInfoNotFoundError at the top of the file.
def get_current_time(city: str) -> dict:
"""
Returns the current time in the specified city.
This is a simplified example and only supports a few cities.
"""
# For example purpose only. Ideally, integrate with a world time API
# or a more comprehensive city-to-timezone mapping.
city_to_tz = {
"new york": "America/New_York",
"london": "Europe/London",
"tokyo": "Asia/Tokyo",
}
tz_name = city_to_tz.get(city.lower())
if not tz_name:
return {
"status": "error",
"message": f"Unknown city: {city}. Supported cities are: {', '.join(city_to_tz.keys())}",
}
try:
tz = ZoneInfo(tz_name)
time_now = datetime.datetime.now(tz).strftime('%I:%M %p')
return {"status": "success", "city": city, "time": time_now}
except ZoneInfoNotFoundError:
# This should not happen with the hardcoded map, but is good practice.
return {"status": "error", "message": f"Invalid timezone configured for {city}: {tz_name}"}
|
Hi @saeedtamboli01 ,Thank you for your contribution! |
ryanaiagent
added
the
awaiting-user-response
There was a problem hiding this comment.
Code Review
This pull request introduces a new agent for retrieving the current time in a specified city. While the agent and its tool are set up correctly, the core function get_current_time has a critical flaw: it ignores the city input and returns the server's local time instead. This makes the agent's functionality incorrect and misleading. My review focuses on this correctness issue.
| Returns the current time in the specified city. | ||
| """ | ||
| # For example purpose only. Ideally, integrate with a world time API. | ||
| time_now = datetime.datetime.now().strftime('%I:%M %p') |
There was a problem hiding this comment.
The current implementation ignores the city parameter and returns the server's local time, which contradicts the function's documentation and the agent's purpose of telling the time in a specific city. To correctly implement this, you should use the city to determine the correct timezone and calculate the corresponding time. A library like pytz could be used for this.
3 participants
Implemented a function to get the current time in a specified city and integrated it into the root agent.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
If applicable, please follow the issue templates to provide as much detail as
possible.
Problem:
A clear and concise description of what the problem is.
Solution:
A clear and concise description of what you want to happen and why you choose
this solution.
Testing Plan
Please describe the tests that you ran to verify your changes. This is required
for all PRs that are not small documentation or typo fixes.
Unit Tests:
Please include a summary of passed
pytestresults.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any
necessary setup or configuration. Please provide logs or screenshots to help
reviewers better understand the fix.
Checklist
Additional context
Add any other context or screenshots about the feature request here.