-
Notifications
You must be signed in to change notification settings - Fork 24
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 container items change event being saved in preferences #196
Conversation
'%s.%s' % (self._get_path(), trait_name), | ||
getattr(self, trait_name) | ||
) | ||
return |
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 ugly, and is similar to what the UI counterpart is doing:
apptools/apptools/preferences/ui/preferences_page.py
Lines 76 to 81 in 83aa787
# If the trait was a list or dict '_items' trait then just treat it as | |
# if the entire list or dict was changed. | |
if trait_name.endswith("_items"): | |
trait_name = trait_name[:-6] | |
if self._is_preference_trait(trait_name): | |
self._changed[trait_name] = getattr(self, trait_name) |
The alternative might have been a redesign of the helper <-> preference relationship, but that might not be feasible without changing how it is used.
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!
No suggested changes, just one observation / question
# If we were the one that set the trait (because the underlying | ||
# preferences node changed) then do nothing. | ||
if self.preferences and self._is_preference_trait(trait_name): | ||
if self._is_preference_trait(trait_name): |
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 likely an unrealistic edge case (it would probably be considered a programmer error), but what if there is a preference trait that ends with _items
, e.g. something_items
? In that case we enter the above if statement, and set trait_name = = trait_name[:-6]
removing the trailing _items
leaving us with something
. Then calling getattr(self, trait_name)
will fail.
In general you should never name traits with a trailing _items
, so this code is good to stay as is. But just wanted to ask about this.
e.g. this test fails:
def test_mutate_list_of_values(self):
""" Mutated list should be saved and _items events not to be
saved in the preferences.
"""
# Regression test for enthought/apptools#129
class MyPreferencesHelper(PreferencesHelper):
preferences_path = Str('my_section')
list_of_str_items = List(Str)
helper = MyPreferencesHelper(list_of_str_items=["1"])
# Now modify the list to fire _items event
helper.list_of_str_items.append("2")
self.preferences.save(self.tmpfile)
new_preferences = Preferences()
new_preferences.load(self.tmpfile)
self.assertEqual(
new_preferences.get("my_section.list_of_str_items"), str(["1", "2"])
)
self.assertEqual(new_preferences.keys("my_section"), ["list_of_str_items"])
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.
Yep, it is a very good observation. I agree this is why magic naming is bad, and one of the reasons to introduce observe
is to do away with _items
magic naming.
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 think the test you provided can be fixed though, but it may have to be done along with apptools.preferences.ui.preferences_page
. I will do that in a separate PR.
To be honest, I think a better design would be one that does not require synchronization - we probably don't need that. Most of the time we care about storing the preferences to a file after it has been edited. As long as we can obtain and serialize all the values at the point when the file is saved, we are good. If someone says they want to share the same preferences with multiple helpers, I'd say, think again. A lot can go wrong when editing shared mutable states, best approach is don't.
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 think the test you provided can be fixed though, but it may have to be done along with apptools.preferences.ui.preferences_page. I will do that in a separate PR.
Actually thinking about it... I am not sure it can be fixed to satisify both situations. The suffix _items
is simply overloaded. It might be one that just needs to be documented and a warning be emitted perhaps.
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.
Opened #225
# If the trait was a list or dict '_items' trait then just treat it as | ||
# if the entire list or dict was changed. | ||
if trait_name.endswith('_items'): | ||
trait_name = trait_name[:-6] |
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.
when we get to python 3.9, we would be able to use removesuffix
method - which would make the code more readable. ref https://www.python.org/dev/peps/pep-0616/
maybe we should add two backwards compatible removeprefix
and removesuffix
functions in a traits utility - which other libraries that depend on traits can use.
Closes #129
This PR fixes the particular situation in #129 where the TraitListEvent is saved in the preferences and that the updated list is not saved in the preferences. This should fix a similar scenario for a Set or Dict. However, this does not fix the situations for nested containers. see: #194. I have added a sort of 'expected-failure' test in this PR.
Reacting to changes in arbitrarily nested containers could be supported by observe. However, not knowing the structure of the traits being serialized in the preferences, one may require recursion support, which has not been implemented in Traits observe.
Checklist