Add version to block script data #122
Conversation
manuq
force-pushed
the
T35547-add-version-to-data
branch
from
8acf1f2 to
e298518
Compare
|
I very much like this idea, but I think we may not want to use the actual plugin version in the script. Instead, I think we should maintain a separate data version. Since the data format won't change with every plugin release, we don't need to attempt migration. This would also handle a development snapshot since the data version could be bumped independently of the plugin version. The data version could be a simple integer counter that we bump whenever the data format changes. We'd set that to 1 now and any script that does not have a version is treated as 0. I don't think we'd need point releases as the semantics would always be to migrate the data to the current version regardless of what it previously was.. |
manuq
force-pushed
the
T35547-add-version-to-data
branch
from
e298518 to
1e59756
Compare
|
@dbnicholson good points. I addressed the comments and force-pushed. |
| if version == Constants.CURRENT_DATA_VERSION: | ||
| # No migration needed. | ||
| return | ||
| push_warning("Migration not implemented from %d to %d" % [version, Constants.CURRENT_DATA_VERSION]) |
There was a problem hiding this comment.
I think this is good enough for now, but in the future we might want to beef this up. If the stored data version is less than the current data version, migrate it. If the stored data version is greater than the current data version, then someone has already migrated the data to a newer plugin version and is now trying to run an older version of the plugin. Since the older plugin version by definition doesn't know about the newer data format, I think the only thing to do there would be to refuse to load the script and bring up some kind of error message.
| @@ -129,6 +141,7 @@ func save_script(): | |||
| var generated_script = _block_canvas.generate_script_from_current_window(block_script.script_inherits) | |||
| block_script.block_trees = block_trees | |||
| block_script.generated_script = generated_script | |||
| block_script.version = Constants.CURRENT_DATA_VERSION | |||
There was a problem hiding this comment.
Can we arbitrarily update the version in the saved script if it hasn't been migrated? This is probably fine for now but eventually I'd expect save_script to bail if _current_block_code_node.version != Constants.CURRENT_DATA_VERSION. Maybe generate_script_from_current_window is the one doing the heavy lifting there and should return null if it can't understand the blocks on screen.
There was a problem hiding this comment.
Yes, this is TBD. I guess we'll try a migration and if that fails the blocks shouldn't be editable at all, can't be saved.
addons/block_code/ui/constants.gd
Outdated
| @@ -1,5 +1,7 @@ | |||
| extends Object | |||
|
|
|||
| const CURRENT_DATA_VERSION = 1 | |||
There was a problem hiding this comment.
Backtracking on my earlier thought, I wonder if this should still be 0 since we haven't changed the serialization yet. #107, for example, would bump this up. Or we just keep it 0 until we're ready to handle stability.
There was a problem hiding this comment.
Yes, let's keep it zero until we are ready to handle migrations.
In the plugin, add a new constant for the current data version. This will start in 0 and be bumped each time the data format changes. In the BlockScriptData resource, add an integer version field which defaults to zero. The version will be set to the current version when the resource is saved. The actual migration is TBD. Only a warning is printed for now. https://phabricator.endlessm.com/T35547
manuq
force-pushed
the
T35547-add-version-to-data
branch
from
1e59756 to
20f5f54
Compare
https://phabricator.endlessm.com/T35547