-
Notifications
You must be signed in to change notification settings - Fork 264
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
make RegisterSerializer generic #4422
make RegisterSerializer generic #4422
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.
Fine by me. @MarcoRossignoli WDYT ?
yeah it just makes the usage a little more succinct |
{ | ||
_typeSerializer.Add(type, namedPipeSerializer); | ||
INamedPipeSerializer namedPipeSerializer = new TSerializer(); |
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.
A minor difference here is that Activator.CreateInstance
is now used to construct namedPipeSerializer
.
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.
@SimonCropp I have discussed it with the team and we will stick with current approach. Activator.CreateInstance
should work with NAOT and trimming but we don't know if there are cases where it would be different. We don't yet have automatized enough tests around NAOT and trimming and we know current approach does work on all manual tests we did.
this is purely cosmetic. so feel free to burn it if u dont like it