-
Notifications
You must be signed in to change notification settings - Fork 992
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
[REF-3009] type transforming serializers #3227
[REF-3009] type transforming serializers #3227
Conversation
b50b3d4
to
752e8b9
Compare
Instead of returning a tuple with the serialized type we could also add a kwarg to the @serialize(to=str)
def serialize_datetime(dt: datetime):
return str(dt) |
Serializers can also return a tuple of `(serialized_value, type)`, if both ways are specified, then the returned value MUST match the `to` parameter. When initializing a new rx.Var, if `_var_is_string` is not specified and the serializer returns a `str` type, then mark `_var_is_string=True` to indicate that the Var should be treated like a string literal. Include datetime, color, types, and paths as "serializing to str" type. Avoid other changes at this point to reduce fallout from this change: Notably, the `serialize_str` function does NOT cast to `str`, which would cause existing code to treat all Var initialized with a str as a str literal even though this was NOT the default before. Update test cases to accomodate these changes.
In the future, we will treat strings as string literals in the JS code. To get a Var that is not treated like a string, pass _var_is_string=False. This will allow our serializers to automatically identify cast string literals with less special cases (and the special cases need to be explicitly identified).
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.
@masenf your changes look good to me, thank you for jumping in!
I am thinking about dropping the tuple
return in favor of the new to
kwarg. If I am not missing any advantages of the tuple
return, I think it would be better to have one way to do things.
The tuple approach would allow dynamic serialization types (type based on input value), but we can't use them in the frontend, so I see no real benefit.
Simplify the logic; instead of making a wrapper function that returns a tuple, just save the type conversions in a separate global.
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.
@masenf thanks for refactoring, looks good to me!
No description provided.