Skip to content

Move implicit output bindings serialization logic to Python library #643

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 5 commits into from
Apr 23, 2020

Conversation

Hazhzeng
Copy link
Contributor

@Hazhzeng Hazhzeng commented Apr 2, 2020

Background

Even when a trigger type is marked with implicit output, it is still possible for an extension to marshal the output result (e.g. Python Durable Function's ActivityTrigger needs to serialize the return result into JSON when it sends to the durable extension).

Justification

Handing over this control to Python Library allows an implicit_output trigger to modify the encode behavior flexibly.

has_return = True
return_binding_name = f'{desc.type}_ret'
has_implicit_return = True
return_binding_name = desc.type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the line we refer to our trigger bindings (a.k.a. python-library activity-trigger) for serializing implicit output values.

Copy link

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

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

Looks good to me! Although I'm not an active owner of this repo so I would prefer to wait for someone else to approve of these changes before merging.

@amamounelsayed
Copy link

The same here, looks good to me. Just if we have someone from durable function have a look that will be great.

Copy link

@AnatoliB AnatoliB left a comment

Choose a reason for hiding this comment

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

LGTM (assuming the CI tests pass)

Copy link

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

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

Also looks good to me! Let's fix those build errors and merge 👍

@Hazhzeng Hazhzeng force-pushed the hazeng/fix-implicit-out branch from 05d6356 to 01c49c0 Compare April 23, 2020 17:51
@Hazhzeng Hazhzeng merged commit 9e70e3a into dev Apr 23, 2020
@Hazhzeng Hazhzeng deleted the hazeng/fix-implicit-out branch April 29, 2020 01:51
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.

4 participants