-
Notifications
You must be signed in to change notification settings - Fork 62
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
Fix settings constructor backward compatibility issue #214
Fix settings constructor backward compatibility issue #214
Conversation
@@ -72,7 +80,25 @@ public class SerializerOptions | |||
internal readonly List<Func<string, string>> CrossFrameworkPackageNameOverrides = | |||
DefaultPackageNameOverrides(); | |||
|
|||
public SerializerOptions(bool versionTolerance = false, bool preserveObjectReferences = false, IEnumerable<Surrogate> surrogates = null, IEnumerable<ValueSerializerFactory> serializerFactories = null, IEnumerable<Type> knownTypes = null, bool ignoreISerializable = false, IEnumerable<Func<string, string>> packageNameOverrides = null) | |||
[Obsolete] | |||
public SerializerOptions( |
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 is the old constructor, added back in for backward compatibility issue.
New constructor broke backward compatibility because it was called using reflection and Activator
class.
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.
IMHO, this is part of the problem with using optional parameters everywhere - very difficult to properly version the constructor when that happens. That's a design we inherited, but we might want to rethink that.
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.
Can you add an Akka.NET integration test that verifies that Hyperion can be loaded correctly inside Akka.Serialization.Hyperion? Might be worth adding a new test project for that purpose.
Will do |
@@ -0,0 +1,30 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> |
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.
Probably need to have this reference common.props
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.
Good point.
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.
LGTM
Closes #212