-
Notifications
You must be signed in to change notification settings - Fork 4.9k
[time] Cross-platform support for device timezones via tzlocal
#925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Hey @michaelfromyeg , thanks for cc :) |
ah all good! |
Thanks for the PR! I'm wondering if we should try to combine this approach and the approach in #1272 to cover all our bases (I left a comment in that one). |
I trust your judgement on which is best! Not sure how much fallback-ing is necessary... but better safe than sorry? Candidly, tzlocal was what came up first when I searched up the issue (via Claude!). |
@michaelfromyeg since your PR includes some doc formatting updates too and was created first, let's do this:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, was running the tests for this locally and ran into some errors related to McpError so I left a few suggestions to resolve those.
Also a suggestion for a test to add that would check for valid vs invalid overrides:
@pytest.mark.parametrize(
"test_time,local_tz_override,expected_error",
[
# Test Windows format timezone override
(
"2024-01-01 12:00:00+00:00",
"America/Los_Angeles", # What Windows "Pacific Standard Time" should map to
None,
),
# Test direct IANA name
(
"2024-01-01 12:00:00+00:00",
"Europe/Berlin", # What "Mitteleuropäische Zeit" should map to
None,
),
# Test invalid timezone
(
"2024-01-01 12:00:00+00:00",
"Invalid/Timezone",
"Invalid timezone: 'No time zone found with key Invalid/Timezone'",
),
],
)
def test_get_local_tz_override(test_time, local_tz_override, expected_error):
"""Test local timezone override functionality"""
with freeze_time(test_time):
if expected_error:
with pytest.raises(McpError, match=expected_error):
get_local_tz(local_tz_override)
else:
result = get_local_tz(local_tz_override)
assert isinstance(result, ZoneInfo)
assert str(result) == local_tz_override
if local_tz_override: | ||
return ZoneInfo(local_tz_override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion to add try/catch if the local override results in an exception, plus it looks like McpError expects an object with a message and not a string:
if local_tz_override: | |
return ZoneInfo(local_tz_override) | |
if local_tz_override: | |
try: | |
return ZoneInfo(local_tz_override) | |
except Exception as e: | |
error_data = ErrorData( | |
code=INVALID_PARAMS, | |
message=f"Invalid timezone: {str(e)}" | |
) | |
raise McpError(error_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will also need to add ErrorData and INVALID_PARAMS fom mcp.types, for example:
from mcp.types import Tool, TextContent, ImageContent, EmbeddedResource, ErrorData, INVALID_PARAMS
tzinfo = datetime.now().astimezone(tz=None).tzinfo | ||
if tzinfo is not None: | ||
return ZoneInfo(str(tzinfo)) | ||
|
||
raise McpError("Could not determine local timezone - tzinfo is None") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise McpError("Could not determine local timezone - tzinfo is None") | |
error_data = ErrorData( | |
code=INVALID_PARAMS, | |
message="Could not determine local timezone - tzinfo is None" | |
) | |
raise McpError(error_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't leave comments or suggestions on unchanged lines, but I think you also want to change this in the get_zoneinfo
function:
except Exception as e:
error_data = ErrorData(
code=INVALID_PARAMS,
message=f"Invalid timezone: {str(e)}"
)
raise McpError(error_data)
I tried to use the time server on Windows, and noticed a timezone error. Windows seems to report the timezone in a strange format...
These kinds of values didn't play nice with
ZoneInfo
. This PR addstzlocal
to fix that.One thing: I couldn't figure out how to test this reliably. Let me know if you'd like me to add tests.
cc @maledorak for viz!
edit: also #320 is probably worth another glance 👀
Description
Server Details
get_local_tz
helperMotivation and Context
zoneinfo._common.ZoneInfoNotFoundError: 'No time zone found with key Pacific Daylight Time'
) viatzlocal
How Has This Been Tested?
Breaking Changes
No
Types of changes
Checklist
Additional context