-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add support for on-the-fly document upgrades #2489
Conversation
This also addresses #2430 |
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.
Looking into the issue for more context, I think is better to @mbdavid to take a look on this and see if he wants to perform this change, or if using the protected
methods are enough
@pictos Thanks for the review, how did that slip through, that its a breaking change 😅 So in my latest commit i added a deserialization hook. Upsides:
Downsides:
|
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'm away from my pc, so I can't test it right now
Co-authored-by: Pedro Jesus <pedrojesus.cefet@gmail.com>
Some tests failed before which are succeding (not the tests i touched)...sooo... @pictos Can we merge? 😅 |
@@ -78,6 +96,15 @@ public T Deserialize<T>(BsonValue value) | |||
/// </summary> | |||
public object Deserialize(Type type, BsonValue value) | |||
{ | |||
if (OnDeserialization is not null) | |||
{ | |||
var result = OnDeserialization(this, type, value); |
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.
To keep consicise with all code base, let's use "this." for instance methods, properties and public fields (only private _ name has no "this.")
This change would allow me to improve my library (https://github.com/JKamsker/LiteDb.Migration) significantly. The documents would then only be upgrades on load and not all at once.