-
Notifications
You must be signed in to change notification settings - Fork 2
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
new shit #6
new shit #6
Conversation
also add a 2.0 migration
…er than archivist's internalversion
at least, attempt to. Every chance that there will be 153 "fixes" following this commit
specifically don't copy things to sv.stores that we expect to find in the SV, but which are definitely not store types
@Meorawr since you've had some experience playing with this before, would you mind giving this a spin? I didn't find any unexpected breakages but I'm famously bad at regression testing. |
Major changes are:
-- assume AddonA, AddonB are both savedvariables that archivist has interacted with before
local Archivist = select(2, ...).Archivist
local first = Archivist(AddonA, {-[[table of store types to register in first archive]]})
local second = Archivist(AddonB)
first:Load("RawData", "jerry's data")
second:RegisterStoreType({-[[snip]]})
-- etc etc, first & second operate totally independently of each other as far as consuming code is concerned
|
}, { | ||
__index = proto | ||
}) | ||
if type(sv.internalVersion) ~= "number" or sv.internalVersion < self.internalVersion then |
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've got a slight concern here that doing migrations upon initialization of the instance may cause script timeout errors for people with gargantuan amounts of data, considering that this first migration will be touching every single store to decompress, deserialize, re-serialize, and re-compress things.
Even if it isn't a significant issue now, any future migrations which do similar things would potentially have a stacking effect in the case where multiple migrations need to be performed.
It might be better to apply this migration on a store-by-store basis at the point when an individual store is opened by checking the internal versioning at a store level to see which deserializer should be used to initially read it.
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 a good point. I think I'll work on this some more in a follow-on change, once rossnichols/LibSerialize#4 & SafeteeWoW/LibDeflate#6 are complete.
& interpret "unversioned" SV as version 0 also tweak an error message to be more informative
Thanks @Meorawr & @InfusOnWoW, I appreciate the extra eyes ❤️ |
I think this counts as a major version bump