-
Notifications
You must be signed in to change notification settings - Fork 34
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
Added Non-Generic Solution to KiotaDeserialization #436
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.
Thanks for the contribution!
src/abstractions/serialization/KiotaJsonSerializer.Deserialization.Untyped.cs
Outdated
Show resolved
Hide resolved
src/abstractions/serialization/KiotaJsonSerializer.Deserialization.Untyped.cs
Outdated
Show resolved
Hide resolved
Okay .. @baywet I was also able, to remove the |
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.
Thank you for making the changes!
A couple additional questions/comments.
src/abstractions/serialization/KiotaJsonSerializer.Deserialization.NonGeneric.cs
Outdated
Show resolved
Hide resolved
src/abstractions/serialization/KiotaJsonSerializer.Deserialization.NonGeneric.cs
Outdated
Show resolved
Hide resolved
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
…s shadowing parent methods Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
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.
Thank you for making the changes!
I also took the liberty of making a bunch of "cleanup" changes here.
@andrueastman for final review and merge
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.
Otherwise looks good from my end. Will update version/release notes on getting clarification on duplicate code question.
tests/abstractions/Serialization/DeserializationHelpersTests.NonGeneric.cs
Outdated
Show resolved
Hide resolved
removes duplicate tests
This code addition fulfills the request of Issue 210, where this was explicitly requested.
@dansmitt is a co-worker of mine and I thought, that this might be of relevance to more than just him.
I used the same Unit-Tests, as you were, modifying them just as much, to use the newly introduced method signatures.