Skip to content

Conversation

@zach-klippenstein
Copy link
Collaborator

@zach-klippenstein zach-klippenstein commented Jun 28, 2020

This simplifies the API of WorkflowAction.applyTo, removes the need for
OutputHolder in the core tests and MaybeOutput in workflow-runtime.
EmittedOutput is left as a deprecated typealias to ease migration.

I was initially reluctant to do something like this because I didn't want
Workflow to provide yet another generic Optional type. I forgot that the
testing library was already doing that though, and I think that using a
nullable "holder" type is a gross enough API that it won't likely be used
outside of the intended uses.

@zach-klippenstein zach-klippenstein added this to the v1.0.0 milestone Jun 28, 2020
@zach-klippenstein zach-klippenstein requested a review from a team as a code owner June 28, 2020 14:40
This simplifies the API of `WorkflowAction.applyTo`, removes the need for
`OutputHolder` in the core tests and `MaybeOutput` in `workflow-runtime`.
`EmittedOutput` is left as a deprecated typealias to ease migration.

I was initially reluctant to do something like this because I didn't want
Workflow to provide yet another generic `Optional` type. I forgot that the
testing library was already doing that though, and I think that using a
nullable "holder" type is a gross enough API that it won't likely be used
outside of the intended uses.
fun setOutput(output: O) {
this.output = output
isOutputSet = true
this.output = WorkflowOutput(output)
Copy link
Collaborator

Choose a reason for hiding this comment

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

OMG so much better.

@zach-klippenstein zach-klippenstein merged commit 15f013a into main Jun 29, 2020
@zach-klippenstein zach-klippenstein deleted the zachklipp/unify-outputs branch June 29, 2020 00:36
zach-klippenstein added a commit that referenced this pull request Jun 29, 2020
After #49 made outputs nullable, and introduced new `RenderTestResult`
methods to handle nullable outputs (`verifyActionState`, `verifyActionOutput`,
and `verifyNoActionOutput`), #64 came along and introduced a dedicated
type for nullable outputs, `WorkflowOutput`. This type means that we can
revert the `RenderTestResult` changes made in #49 and return to the simpler
API with only `verifyAction` and `verifyActionResult` methods. This change
does exactly that.
zach-klippenstein added a commit that referenced this pull request Jun 29, 2020
After #49 made outputs nullable, and introduced new `RenderTestResult`
methods to handle nullable outputs (`verifyActionState`, `verifyActionOutput`,
and `verifyNoActionOutput`), #64 came along and introduced a dedicated
type for nullable outputs, `WorkflowOutput`. This type means that we can
revert the `RenderTestResult` changes made in #49 and return to the simpler
API with only `verifyAction` and `verifyActionResult` methods. This change
does exactly that.
zach-klippenstein added a commit that referenced this pull request Feb 4, 2021
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.

3 participants