Skip to content

Advanced type hinting support #102

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

Merged
merged 3 commits into from
Aug 12, 2022
Merged

Advanced type hinting support #102

merged 3 commits into from
Aug 12, 2022

Conversation

cretz
Copy link
Member

@cretz cretz commented Aug 9, 2022

What was changed

  • Removed dacite
  • Wrote custom type-hint support for deserializing

Why?

Need to support all sorts of type hints like Optional, Union, etc

Checklist

  1. Closes Improve dataclass type hinting #78

@cretz cretz requested a review from a team August 9, 2022 13:41
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

Looks like that wasn't too bad

Comment on lines +809 to +810
This is used internally to convert a raw JSON loaded value to a specific
type hint.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this more like?

Suggested change
This is used internally to convert a raw JSON loaded value to a specific
type hint.
This is used internally to convert a raw JSON loaded value to an instance
of a specific type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
This is used internally to convert a raw JSON loaded value to a specific
type hint.
This is used internally to convert a raw JSON loaded value using a specific
type hint.

That may be clearer. Will change if there is another commit here.

@cretz
Copy link
Member Author

cretz commented Aug 9, 2022

Looks like that wasn't too bad

@Sushisource - It wasn't too bad, but I fear people may have custom needs and it may be hard to document in detailed form what we specifically support. Also, while the code is small, it may not be obvious to users that, for example, a union tries all union types and chooses the first one (so if the first dataclass matched I wouldn't try the second).

Copy link
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

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

Tests look good didn't look into the implementation too much.

@cretz cretz merged commit efc1c7b into temporalio:main Aug 12, 2022
@cretz cretz deleted the type-hints branch August 12, 2022 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve dataclass type hinting
3 participants