Skip to content
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

Clean up PrefixList and PrefixMap #1564

Merged
merged 11 commits into from
Oct 5, 2021
Merged

Conversation

mdickinson
Copy link
Member

This PR does some refactoring and cleanup on the PrefixList and PrefixMap trait types. It's a more careful re-invention of the changes in #1563 (extended to PrefixMap).

Principal user-facing changes:

  • The cache for value completion, which was questionable from a thread-safety point of view (see PrefixList and PrefixMap are not thread-safe #1561) has been removed, but validation for a value that's in the list / map rather than just a prefix will still be O(1), not O(n). Validation for a value that has to be completed will now be O(n) in every case rather than being O(n) on the first use and O(1) thereafter. However, we're not expecting these trait types to be used with value lists big enough for this to make a difference.
  • A bad static default value in the trait type definition will now consistently raise ValueError rather than TraitError.
  • The default_value is now explicit in the trait type signature, rather than being extracted from the metadata. (This doesn't change any behaviour, but does improve the documentation and discoverability.)
  • The nesting of exceptions is fixed (see Nested exceptions from failed PrefixList validation. #955).

Fixes #955.
Fixes issues described in #1561.

@mdickinson mdickinson mentioned this pull request Oct 4, 2021
@rahulporuri rahulporuri self-requested a review October 5, 2021 08:32
traits/trait_types.py Outdated Show resolved Hide resolved
traits/trait_types.py Outdated Show resolved Hide resolved
@@ -3141,7 +3156,7 @@ class Map(TraitType):

The following example defines a ``Person`` class::

>>> class Person(HasTraits):
>>> erson(HasTraits):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something odd has happened to this line.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks; should be fixed in e2435a5.

@@ -3316,7 +3348,7 @@ def post_setattr(self, object, name, value):
setattr(object, name + "_", self.mapped_value(value))

def info(self):
keys = sorted(repr(x) for x in self.map.keys())
keys = sorted(repr(x) for x in self.map)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a difference between PrefixList and PrefixMap here: we sort the output for PrefixMap but not PrefixList. Given that we can now rely on ordered dictionaries, I think we should drop the sorted here.

Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue (fixed in e2435a5), otherwise LGTM.

@rahulporuri rahulporuri removed their request for review October 5, 2021 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested exceptions from failed PrefixList validation.
2 participants