[ENH] Setting migration #1724
[ENH] Setting migration #1724
Conversation
Add a helper method that provides pickled setting defaults (by patching open)
Current coverage is 88.86% (diff: 100%)@@ master #1724 diff @@
==========================================
Files 82 82
Lines 8804 8804
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 7824 7824
Misses 980 980
Partials 0 0
|
| @@ -738,6 +745,32 @@ def _userconfirmed(): | |||
|
|
|||
| self.__msgwidget.accepted.connect(_userconfirmed) | |||
|
|
|||
| @classmethod | |||
| def migrate_settings(cls, settings, version): | |||
There was a problem hiding this comment.
Doesn't this method belong to OWComponent? Settings belong to OWComponents and as of be3ff05 (see changes in widget.py), they will be implemented there and no longer in OWWidget.
If we merge this PR as it is, I'm not sure that components will work (haven't tried, though). Instead of duplicating the method in OWComponent, maybe we should merge #1665 first and then move this method to OWComponent.
There was a problem hiding this comment.
This belongs to whoever defines the settings handler (the class passed in parameter to SettingHandler.create call). As components can be used as a part of the widget, they do not have their own settingHandlers and reuse the widget's. The widget is therefore responsible for migration of all the settings.
There was a problem hiding this comment.
The widget is therefore responsible for migration of all the settings.
Say that some component is reused in multiple widgets. If we change a setting in the component, we have to add the corresponding migration to all widgets? Including, say, widgets in third-party add-ons that use our components? With the later, any change in component's settings would break compatibility with widgets; with introducing component-level migrations, this can be avoided.
Wouldn't it thus be better if settings handler traversed all components (including the widget itself) and called the corresponding migration method in each? This would also require setting versioning on components, not widgets.
There was a problem hiding this comment.
If the settings break with a change, so might the widget that uses the component. If for instance a component used to store a list of indices in a setting and now it stores a single number, a widget trying to index the appropriate component attribute will crash.
While I can move the migrate_settings method to the component, what can we do with migrate_context? Leave it on the widget? If this is the case, I would prefer to keep the methods in the same place. And if the migrations are placed on a component, who defines the settings version?
IMO, if component is complicated enough to warrant its own migration, it can define its own migrate_settings, migrate_context, but the widget should be aware of that and call the method explicitly in its own migrate_settings method with proper parameters.
astaric
force-pushed
the
setting-migration
branch
2 times, most recently
from
1d50057
to
944d112
Compare
astaric
force-pushed
the
setting-migration
branch
from
944d112
to
386ce93
Compare
|
I have added a mention of migrations to the widget development tutorial. |
Issue
When widgets are opened with old version of settings, they break in many different ways.
Description of changes
This PR introduces the concept of setting versions and migrations. When a widget changes its settings in a way that is not compatible with old version, it should raise its settings_version attribute and implement the migrate_settings (migrate_context for context settings) method.
Includes