-
Notifications
You must be signed in to change notification settings - Fork 922
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
Set TypeId for structured types at decode. #1260
Conversation
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 don't see functional changes when the new parameter is not used, so this is ok.
But the interface change causes a build error if not being addressed, so version json should be bumped to 366 due to the interface change. Please add it, then its all fine.
-- just some head scratching. If it can be recompiled to the new libs without change we can stick with 365...
/// <param name="systemType">The system type of the encopdeable object to be read</param> | ||
/// <param name="encodeableTypeId">The TypeId for the <see cref="IEncodeable"/> instance that will be read.</param> | ||
/// <returns>An <see cref="IEncodeable"/> object that was read from the stream.</returns> | ||
IEncodeable ReadEncodeable(string fieldName, System.Type systemType, ExpandedNodeId encodeableTypeId = 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.
looks like a breaking change to IDecoder interface. We need to bump up version json to 366.
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.
or is a caller able to consume it without code change? Then we can stick with 365
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.
It should be no breaking change since since default value is set encodeableTypeId = 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.
@AlinMoldovean, then it looks good to me.
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.
great, then its all good
No description provided.