-
Notifications
You must be signed in to change notification settings - Fork 62
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
Feature/datetime conversions #174
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ | |
import google.protobuf.json_format | ||
import google.protobuf.message | ||
import google.protobuf.symbol_database | ||
from dateutil import parser | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a new dependency just for this feature. Is there any way we can avoid it? Is there any way with the standard library to do this, and/or is the parser easy to vendor/copy into here? (also, a new dependency would need to be added to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like it may be unavoidable to add this dependency unfortunately. Can't easily vendor stdlib impl as it's in C (ref python/cpython#92177). But can we put a Also, this will need to land after (and merge main after) #172 is merged which will add Python 3.11 support. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not an issue on my side, I must have been working on a tainted env because I thought For those wondering about why this is an issue: On doing it within the standard library, we could hack around with the following:
There's a slight catch here that "J" = local time, so we'd have to decide if we treat that string as timezone unaware or pull the timezone from the local machine. The only reason I avoided this was to rely on a battle-tested date parser that could potentially catch a wider range of formats coming in from other languages. If you're confident we'll only see letters coming in (outside of the +/- hours format) then I'm happy to switch over to an explicit remap of the last digits for the sake of avoiding the external dependency There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Roger that, will add in the version check and wait for the 3.11 updates you guys have planned |
||
from typing_extensions import Literal | ||
|
||
import temporalio.api.common.v1 | ||
|
@@ -697,7 +698,7 @@ def decode_search_attributes( | |
val = [val] | ||
# Convert each item to datetime if necessary | ||
if v.metadata.get("type") == b"Datetime": | ||
val = [datetime.fromisoformat(v) for v in val] | ||
val = [parser.isoparse(v) for v in val] | ||
ret[k] = val | ||
return ret | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
import sys | ||
from collections import deque | ||
from dataclasses import dataclass | ||
from datetime import datetime | ||
from datetime import datetime, timezone | ||
from enum import Enum, IntEnum | ||
from typing import ( | ||
Any, | ||
|
@@ -32,6 +32,7 @@ | |
import temporalio.common | ||
import temporalio.converter | ||
from temporalio.api.common.v1 import Payload as AnotherNameForPayload | ||
from temporalio.api.common.v1.message_pb2 import Payload | ||
|
||
|
||
class NonSerializableClass: | ||
|
@@ -164,6 +165,37 @@ def test_encode_search_attribute_values(): | |
temporalio.converter.encode_search_attribute_values(["foo", 123]) | ||
|
||
|
||
def test_decode_search_attributes(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to also add an integration test that sets a datetime attribute in a workflow? Or maybe alter There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leave it with me |
||
"""Tests decode from protobuf for python types""" | ||
|
||
def payload(key, dtype, data, encoding=None): | ||
if encoding is None: | ||
encoding = {"encoding": b"json/plain"} | ||
check = Payload( | ||
data=bytes(data, encoding="utf-8"), | ||
metadata={"type": bytes(dtype, encoding="utf-8"), **encoding}, | ||
) | ||
return temporalio.api.common.v1.SearchAttributes(indexed_fields={key: check}) | ||
|
||
# Check basic keyword parsing works | ||
kw_check = temporalio.converter.decode_search_attributes( | ||
payload("kw", "Keyword", '"test-id"') | ||
) | ||
assert kw_check["kw"][0] == "test-id" | ||
|
||
# Ensure original DT functionality works | ||
dt_check = temporalio.converter.decode_search_attributes( | ||
payload("dt", "Datetime", '"2020-01-01T00:00:00"') | ||
) | ||
assert dt_check["dt"][0] == datetime(2020, 1, 1, 0, 0, 0) | ||
|
||
# Check timezone aware works as server is using ISO 8601 | ||
dttz_check = temporalio.converter.decode_search_attributes( | ||
payload("dt", "Datetime", '"2020-01-01T00:00:00Z"') | ||
) | ||
assert dttz_check["dt"][0] == datetime(2020, 1, 1, 0, 0, 0, tzinfo=timezone.utc) | ||
|
||
|
||
NewIntType = NewType("NewIntType", int) | ||
MyDataClassAlias = MyDataClass | ||
|
||
|
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.
Why did so much of this file change? Not that it seems wrong, just doesn't seem like the issue to do this in.
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.
Auto-sorting...I noticed you asked to re-alphabetise, I didn't realise the autoformatter did so much! Will revert
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.
Sorry, I meant just the dependency, heh