Support overriding state options #193
Conversation
Codecov Report
@@ Coverage Diff @@
## main #193 +/- ##
============================================
+ Coverage 71.17% 71.36% +0.18%
- Complexity 362 366 +4
============================================
Files 59 59
Lines 1523 1526 +3
Branches 132 135 +3
============================================
+ Hits 1084 1089 +5
+ Misses 369 368 -1
+ Partials 70 69 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| * @param stateOptions required, to override the defined ones in the State class | ||
| * @return state decision | ||
| */ | ||
| public static StateDecision singleNextState(final Class<? extends WorkflowState> stateClass, final WorkflowStateOptions stateOptions) { |
There was a problem hiding this comment.
We probably have to remove this one, because it's going to be ambiguous with singleNextState(final Class<? extends WorkflowState> stateClass, final Object stateInput). The behavior is undefined with this ambiguity (different based on JVMs). The pattern with stateInput is much more popular so we should keep the other one
There was a problem hiding this comment.
Which JVM will have errors?
I think Java is a storng-typed langauge so that it will choose the most specific method. It means when the second parameter's type is not of WorkflowStateOptions, it will treat the parameter as input?
There was a problem hiding this comment.
Oh I think you are probably right about jvm. The ambiguous case probably won’t happen in this case. (Though it happened before with vararg when I worked on other projects)
But I think it is still better to remove this because it’s not worth having this confusing method here. Also like you mentioned, there are too many of them. The use case of using stateOptionOverride is very rare
| * | ||
| * @param stateClass required | ||
| * @param stateInput required | ||
| * @param stateOptions required, to override the defined ones in the State class |
There was a problem hiding this comment.
It's better to stateOptions => stateOptionsOverride to be more clear
There was a problem hiding this comment.
also other methods in this file
There was a problem hiding this comment.
I think we should say "optional" for stateInput and stateOptionsOveride, because they can be null. Or to be more clear optional, can be null
There was a problem hiding this comment.
We have other methods public static StateDecision singleNextState(final Class<? extends WorkflowState> stateClass ...) without the input or option parameter. In the case of any of them being null, users should call these methods will less parameters?
There was a problem hiding this comment.
Yeah user should call that one. I just feeling like the word “required” is a bit confusing here as you can pass a “null”. Or you can put “can be null” to be clear
There was a problem hiding this comment.
basically, if it's "required" because it's a parameter of the method, then I think this information is not useful -- user already know it's "required" by the compiler/IDE already.
So I think you can change it to "optional", or just say "can be null", without saying it's optional or required,
| @@ -10,6 +11,7 @@ public abstract class StateMovement { | |||
| public abstract String getStateId(); | |||
|
|
|||
| public abstract Optional<Object> getStateInput(); | |||
| public abstract Optional<WorkflowStateOptions> getStateOptions(); | |||
There was a problem hiding this comment.
same here getStateOptionsOverride, and other methods in this file
| return ImmutableStateMovement.builder().stateId(stateId) | ||
| .stateInput(stateInput) | ||
| .build(); | ||
| public static StateMovement create(final Class<? extends WorkflowState> stateClass, final WorkflowStateOptions stateOptions) { |
There was a problem hiding this comment.
same here, this will be ambiguous for JVM. stateOptionsOverride is not a popular pattern, so we should keep the one with stateInput
| * @param stateIds stateIds of next states | ||
| * @return state decision | ||
| */ | ||
| public static StateDecision multiNextStates(final String... stateIds) { | ||
| final ArrayList<StateMovement> stateMovements = new ArrayList<StateMovement>(); | ||
| Arrays.stream(stateIds).forEach(id -> { | ||
| stateMovements.add(StateMovement.create(id)); | ||
| stateMovements.add(StateMovement.create(id, null, null)); |
There was a problem hiding this comment.
I guess we don't have to pass in null, null
There was a problem hiding this comment.
I only keep one method with id because I think it's not commonly used and there are too many methods now. Maybe a better way is to add another method with id only.
There was a problem hiding this comment.
Oh I see. Yeah agreed, I am okay to remove keep this one only.
zklgame
force-pushed
the
zklgame/support_overriding_state_options
branch
from
5f0f506
to
11b7a8c
Compare
This MR did:
StateMovement.create.createmethods with the parameterfinal Class<? extends WorkflowState> stateClasssince they are more commonly used.