-
Notifications
You must be signed in to change notification settings - Fork 17
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
Refactoring: shared utilities and cleanup #313
Conversation
5e22a50
to
cf9c841
Compare
@Override | ||
public void unblock(String message) { | ||
childWorkflowUnblockMessage = message; | ||
} |
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.
The diff here is mostly indentation, moving an impl into the interface - easier to understand with hide-whitespace on.
@@ -5,52 +5,66 @@ | |||
import io.temporal.sdkfeatures.Feature; |
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.
The diff in the file is easier to understand with hide-whitespace on: spotless converting 4-space to 2-space indentation.
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.
Greatly appreciated! I think on the Java SDK there is an auto-formatter. I wonder if we want to bring that over here.
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.
You do have the "spotless" autoformatter active here, which is what made these changes. I guess it tolerates both 4- and 2-space indentation depending on context and I caused it to flip. But I thought I'd let it do its thing because it seems we mostly use 2 spaces.
Now that we're adding more multiple-workflow features, I briefly looked into changing the Java harness so that we can define multiple workflows symmetrically (rather than having one standalone workflow interface and the other workflow interface implemented by the feature), but it's fairly deeply engrained in the harness design:
so I left that. |
@@ -5,52 +5,66 @@ | |||
import io.temporal.sdkfeatures.Feature; |
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.
Greatly appreciated! I think on the Java SDK there is an auto-formatter. I wonder if we want to bring that over here.
Thanks for reviewing @cretz! |
This is pure refactoring aimed at slightly reducing friction when adding new features:
New harness utilities for starting a workflow with default options:
start_parameterless_workflow
andexecuteParameterlessWorkflow
for Python and Typescript respectively.Convert
child_workflow/result
Java feature to use singlefeature.java
file (and moveChildWorkflow
impls insideChildWorkflow
interface)Make use of
runner.executeSingleParameterlessWorkflow
in a Java feature that wasn't using it.