Support string subtypes as function arguments in workflows #261
Conversation
|
|
| raise TypeError( | ||
| f"Cannot convert to {hint}, value not a string, value is {type(value)}" | ||
| ) | ||
| return hint(value) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think it's reasonable to do this for both string and int subtypes. There are two reasons:
class ExampleEnum(str, Enum)was the recommended approach for making json-safe enums prior to the introduction ofStrEnumin 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.- 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.
There was a problem hiding this comment.
👍 Makes sense and looks good. Let's make a few more changes:
- Support that
intsubtype - 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
strandintsubtypes - Format the source
There was a problem hiding this comment.
Thanks! I will do that (though not likely this week).
|
@babymastodon - in #269 I added a way to customize JSON type conversion for any particular type including this one. Is that good enough to solve this issue? Can I close the PR? |
|
Closing PR after no response. Feel free to reopen as necessary. |
3 participants
What was changed
[Regression fix] Support string subtypes as function arguments in workflows, by handling explicitly in
converter.value_to_typeWhy?
When we upgraded from b0.1 to v1.0, we experienced a regression in the deserialization of string subtypes. Not sure exactly which diff introduced the bug. As a result, String subtypes were being deserialized as lists of characters, rather than strings.
Checklist
Closes: N/A
How was this tested: Unit test
Any docs updates needed? Not sure