OpenEMS
(bugfix) Statemachine misses to call onEntry() after initialisation
Directly after initialisation the statemachine misses to call the onEntry handler. This PR fixes this.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Additional note: I had a discussion with @DerStoecki about this PR. Within OpenEMS the current way is to initialize the statemachine with a state "Undefined" and later switch to a new state X. This works, as it always correctly calls X.onEntry(). This is also a "workaround" for using the statemachine. Nevertheless, in several situations I do not want to have an additional state (UndefinedHandler). A statemachine is core functionality, therefore I expect it to properly handle onEntry() and onExit(). I looked for a more elegant way to implement the fix, but was not able to find it. @DerStoecki will try to provide a JUNIT test for this.
Thank you for that pull request. It is rather small in size, but it has the potential to break other state machines out there if they rely on the old behavior.
I see that the new behavior would be the intuitive one. However, I'd do the change rather gradually and add a deprecation warning about that change.
@hydroid7 Thanks for your input. I checked the publically available code before the PR and could not find any break in existing code. Only a few components use the onEntry()/onExit() functions, but none of the existing statemachines uses it in its initial state. So this should not have any impact on the current code base. Nevertheless, there may be implementations out there, which would be impacted by the new behavior. Therefore the deprecation warning is a good idea. Still I am not sure how to practically act it in this situation. How do we do this? Instead of adding a deprecation warning we could rename onEntry() to onEnter(). Thus we can double check all existing code and break cloned code and therefore force others to act on the change. What do you think about this way?
@hydroid7 Thanks for your input. I checked the publically available code before the PR and could not find any break in existing code. Only a few components use the onEntry()/onExit() functions, but none of the existing statemachines uses it in its initial state. So this should not have any impact on the current code base. Nevertheless, there may be implementations out there, which would be impacted by the new behavior. Therefore the deprecation warning is a good idea. Still I am not sure how to practically act it in this situation. How do we do this? Instead of adding a deprecation warning we could rename onEntry() to onEnter(). Thus we can double check all existing code and break cloned code and therefore force others to act on the change. What do you think about this way?
How about:
/**
* @deprecated use {@link #onEnter(CONTEXT)} instead.
*/
@Deprecated
public void onEntry(CONTEXT context) {
log.warn("onEntry() is deprecated. Use onEnter() instead.");
onEnter(context);
}
?
I think it is a good idea, since it allows transition from the old behavior. It is important that the mental model for state machine corresponds to the implementation.
How about:
/** * @deprecated use {@link #onEnter(CONTEXT)} instead. */ @Deprecated public void onEntry(CONTEXT context) { log.warn("onEntry() is deprecated. Use onEnter() instead."); onEnter(context); }
Unfortunately it does not work that way, because the concrete StateHandler implementation extends the abstract StateHandler and overwrites the onEntry() method. Therefore no log warning is generated. And also the redirect is not possible. We would need to place the log warning inside the AbstractStateMachine. But then it gets annoying because everyone will get it.
I could globally refactor the onEntry() call to an onEnter() call from the statemachine and leave an appropriate comment on the onEnter() method. If other repositories follow our coding guidelines they get a build warning because of their own @Override onEntry() implementation. If they had missed the @Override annotation, they will have bad luck and may miss the important change. I am still not lucky with this solution, but we need to take care of the benefit to cost ratio. Don't think that there are many hidden implementations of the state machine out there.
I don't even think there are any StateHandler implementations out there, that would be affected by the change as it is now.
So personally I would simply leave it like it is.
This PR has been automatically marked as stale due to inactivity. It will be closed in 7 days if no further activity occurs.
This PR has been closed due to inactivity
It was automatically closed because there has been no recent activity. If the PR is still relevant, please feel free to reopen and update it and add any new information.