-
Notifications
You must be signed in to change notification settings - Fork 53
Add HasState and default value to GetState<T> #222
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
Conversation
| { | ||
| return dispatcher.DispatchAsync(operation => | ||
| { | ||
| if (operation.State.GetState(typeof(int)) is null) |
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.
This shows how to do this before any of the new APIs. If we think this is good enough, can always revert the new API changes 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.
Using the new APIs would make more sense here I think.
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 changes here are to make it a bit more robust when it is accessed multiple times with different types. Today the following would throw:
operation.State.GetState(typeof(object));
operation.State.GetState(typeof(int));With this change, it will be possible.
sebastianburckhardt
left a comment
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.
some minor suggestions, otherwise looks good to me.
| public abstract class TaskEntityState | ||
| { | ||
| /// <summary> | ||
| /// Gets a value indicating whether this entity has state or not yet. |
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.
| /// Gets a value indicating whether this entity has state or not yet. | |
| /// Gets a value indicating whether this entity has state or not yet / anymore. |
| public void Rollback() | ||
| { | ||
| this.CurrentState = this.checkpointValue; | ||
| this.cachedValue = null; |
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.
I agree that clearing the cache explicitly seems like a good idea here.
(since the code may have modified the object without calling SetState and then the comparison in the setter does not catch that it was modified).
| { | ||
| return dispatcher.DispatchAsync(operation => | ||
| { | ||
| if (operation.State.GetState(typeof(int)) is null) |
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.
Using the new APIs would make more sense here I think.
This PR does the following:
Counterentity.bool HasStatetoTaskEntityState, so there is a quick way to know if an entity has any serialized state or not.T? defaultValueto.GetState<T>to make the new entity scenario easier (won't have any state).TaskEntityShim.StateShima bit to be more robust with multiple state fetches, particularly when/if they change type.