Skip to content

Support string subtypes as function arguments in workflows #261

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Support string subtypes as function arguments in workflows #261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d1b23d
progress
babymastodon Jan 21, 2023
52b5066
progress
babymastodon Jan 21, 2023
0aad2ef
add str enum old style case
babymastodon Jan 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions temporalio/converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1277,6 +1277,14 @@ def value_to_type(hint: Type, value: Any) -> Any:
)
return hint(value)

# String Subtype
if inspect.isclass(hint) and issubclass(hint, str):
if not isinstance(value, str):
raise TypeError(
f"Cannot convert to {hint}, value not a string, value is {type(value)}"
)
return hint(value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrmm, what if they changed the constructor/initializer in the extension? What about extending all other built in types like int, dict, list, etc? Are we sure we want to support everyone's custom class just because it extends a built in we know? It might have just been a coincidence it happened to work before.

At the least, if we do support this, we should update the README explicitly stating we do.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's reasonable to do this for both string and int subtypes. There are two reasons:

  1. class ExampleEnum(str, Enum) was the recommended approach for making json-safe enums prior to the introduction of StrEnum in python 3.11, which is not necessarily deployed everywhere. (Note, by "json-safe", I mean that they are safe to use as keys in a Dict that is serialized to JSON). I've added another failing test case to this PR to illustrate this point.
  2. String subclass is a useful alternative to NewType('TypeName', str), in situations where you are working with libraries that don't play nice with NewType.

I don't really have a strong opinion about Dict/List subtypes. It might make sense to support defaultdict if it's not supported already.

Happy to make more changes if we can agree on the direction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Makes sense and looks good. Let's make a few more changes:

  • Support that int subtype
  • Add an example of a non-string enum failing (just to ensure going forward we don't work with those)
  • Add a bullet "Data Conversion" saying that we support str and int subtypes
  • Format the source

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I will do that (though not likely this week).


# UUID
if inspect.isclass(hint) and issubclass(hint, uuid.UUID):
return hint(value)
Expand Down
11 changes: 11 additions & 0 deletions tests/test_converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,13 @@ class SerializableStrEnum(StrEnum):
FOO = "foo"


class StringSubtype(str):
pass

class StringEnumOldStyle(str, Enum):
FOO = "foo"


@dataclass
class MyDataClass:
foo: str
Expand Down Expand Up @@ -363,6 +370,10 @@ def fail(hint: Type, value: Any) -> None:
[SerializableStrEnum.FOO, SerializableStrEnum.FOO],
)

# String Subtype
ok(StringSubtype, StringSubtype("abc"))
ok(StringEnumOldStyle, StringEnumOldStyle.FOO)

# 3.10+ checks
if sys.version_info >= (3, 10):
ok(list[int], [1, 2])
Expand Down