Conversation
|
|
| update: temporalio.workflow.UpdateMethodMultiParam[MultiParamSpec, ReturnType], | ||
| *, | ||
| args: MultiParamSpec.args, # type: ignore | ||
| wait_for_stage: Literal[temporalio.client.WorkflowUpdateStage.ACCEPTED], |
There was a problem hiding this comment.
Why are we requiring they pass a specific literal that we already know?
There was a problem hiding this comment.
@Quinn-With-Two-Ns has more context, but the gist is that ACCEPTED is an unintuitive default that has historically confused users, so we want to make them aware of the implications of the option (i.e. the worker needs to be running).
There was a problem hiding this comment.
I don't think a nonintuitive default is really a good reason for a parameter, it's a good reason for a docstring. I don't think making every user and callsite provide this argument so that some users don't miss a docstring.
There was a problem hiding this comment.
Discussed offline. We plan to make ADMITTED the default in the future and want to avoid breaking users later.
| ) -> temporalio.client.WorkflowUpdateHandle[Any]: | ||
| # Annotations are not enforced at runtime, so validate anyway. The cast widens the | ||
| # narrowed Literal; without it the check reads as unreachable to the type checker. | ||
| if cast(Any, wait_for_stage) != temporalio.client.WorkflowUpdateStage.ACCEPTED: |
There was a problem hiding this comment.
IMO we should cast to the enum rather than Any. Alternatively we can just add # type: ignore instead of casting. We might also consider allowing this internal method to accept the regular enum and requiring the literal on the public API as you've got here.
There was a problem hiding this comment.
Thanks for the suggestion! Updated the param type to WorkflowUpdateStage.
What was changed
Expose and require the
wait_for_stageparameter to be explicitly set toACCEPTEDfor Nexus Workflow Updates, instead of being hardcoded internally.Why?
wait_for_stageis expected to be a required parameter becauseACCEPTEDis a non-obvious default that users should be aware of. This also aligns with the behaviour in the Go, Java, and .NET SDKs.How was this tested
Add new test.