Skip to content
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

Merged
merged 2 commits into from
Jul 17, 2023

Conversation

dandavison
Copy link
Contributor

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 and executeParameterlessWorkflow for Python and Typescript respectively.

  • Convert child_workflow/result Java feature to use single feature.java file (and move ChildWorkflow impls inside ChildWorkflow interface)

  • Make use of runner.executeSingleParameterlessWorkflow in a Java feature that wasn't using it.

@dandavison dandavison force-pushed the dan-shared-utilities-and-cleanup branch from 5e22a50 to cf9c841 Compare July 15, 2023 19:08
@Override
public void unblock(String message) {
childWorkflowUnblockMessage = message;
}
Copy link
Contributor Author

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;
Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

@dandavison
Copy link
Contributor Author

dandavison commented Jul 15, 2023

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:

this.metadata = POJOWorkflowImplMetadata.newInstance(factoryClass);

so I left that.

@@ -5,52 +5,66 @@
import io.temporal.sdkfeatures.Feature;
Copy link
Member

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.

@dandavison
Copy link
Contributor Author

Thanks for reviewing @cretz!

@dandavison dandavison merged commit 8e1e6cb into main Jul 17, 2023
13 checks passed
@dandavison dandavison deleted the dan-shared-utilities-and-cleanup branch July 17, 2023 15:19
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.

2 participants