From fa767177a4d8da3b1783511d4e6d04f772d718e3 Mon Sep 17 00:00:00 2001 From: Neil Vyas Date: Mon, 19 Feb 2018 15:26:40 -0800 Subject: [PATCH 1/6] Cleaning up how 'type directives' are parsed and set to fix a bug with using an Enum as the type for 'pvector_field'; introducing '_compat.py' --- pyrsistent/_checked_types.py | 52 +++++++++++++++++++++++++++++------- pyrsistent/_compat.py | 9 +++++++ pyrsistent/_field_common.py | 11 +++----- 3 files changed, 55 insertions(+), 17 deletions(-) create mode 100644 pyrsistent/_compat.py diff --git a/pyrsistent/_checked_types.py b/pyrsistent/_checked_types.py index 375f3ac..6d3a9a0 100644 --- a/pyrsistent/_checked_types.py +++ b/pyrsistent/_checked_types.py @@ -1,5 +1,7 @@ from collections import Iterable import six + +from pyrsistent._compat import Enum, string_types from pyrsistent._pmap import PMap, pmap from pyrsistent._pset import PSet, pset from pyrsistent._pvector import PythonPVector, python_pvector @@ -45,18 +47,50 @@ def __str__(self): missing_fields=', '.join(self.missing_fields)) +_preserved_iterables = ( + Enum, +) +"""For types that are themselves iterable, we would mistakenly take their +elements for the type annotation. This set defines types to 'preserve' by not +iterating over them.""" + + +def maybe_type_to_list(t): + """Try to coerce a user-supplied type directive into a list of types.""" + # TODO FIXME: Move type validation inside this function to consolidate it. + # Preserve type annotations that are thmeselves iterable. + if isinstance(t, type) and issubclass(t, _preserved_iterables): + return [t] + # Preserve type annotations given as strings. + elif isinstance(t, string_types): + return [t] + else: + if not isinstance(t, Iterable): + return [t] + else: + return list(t) + + +def maybe_types_to_list(ts): + res = [] + + for t in ts: + res.extend(maybe_type_to_list(t)) + + return tuple(res) + + def _store_types(dct, bases, destination_name, source_name): - def to_list(elem): - if not isinstance(elem, Iterable) or isinstance(elem, six.string_types): - return [elem] - return list(elem) - - dct[destination_name] = to_list(dct[source_name]) if source_name in dct else [] - dct[destination_name] += sum([to_list(b.__dict__[source_name]) for b in bases if source_name in b.__dict__], []) - dct[destination_name] = tuple(dct[destination_name]) - if not all(isinstance(t, type) or isinstance(t, six.string_types) for t in dct[destination_name]): + maybe_types = maybe_types_to_list([ + d[source_name] + for d in ([dct] + [b.__dict__ for b in bases]) if source_name in d + ]) + + if not all(isinstance(t, type) or isinstance(t, string_types) for t in maybe_types): raise TypeError('Type specifications must be types or strings') + dct[destination_name] = maybe_types + def _merge_invariant_results(result): verdict = True diff --git a/pyrsistent/_compat.py b/pyrsistent/_compat.py new file mode 100644 index 0000000..9ae6069 --- /dev/null +++ b/pyrsistent/_compat.py @@ -0,0 +1,9 @@ +from six import string_types + + +# enum compat +try: + from enum import Enum +except: + class Enum(object): pass + # no objects will be instances of this class diff --git a/pyrsistent/_field_common.py b/pyrsistent/_field_common.py index bec3f84..4b28733 100644 --- a/pyrsistent/_field_common.py +++ b/pyrsistent/_field_common.py @@ -1,21 +1,16 @@ from collections import Iterable import six + +from pyrsistent._compat import Enum from pyrsistent._checked_types import ( CheckedType, CheckedPSet, CheckedPMap, CheckedPVector, optional as optional_type, InvariantException, get_type, wrap_invariant, _restore_pickle, get_type) -try: - from enum import Enum as _Enum -except: - class _Enum(object): pass - # no objects will be instances of this class - - def isenum(type_): try: - return issubclass(type_, _Enum) + return issubclass(type_, Enum) except TypeError: return False # type_ is not a class From dcf96b4a65c8af0ca89a0a22f3716d4c1b028fa0 Mon Sep 17 00:00:00 2001 From: Neil Vyas Date: Mon, 19 Feb 2018 15:26:49 -0800 Subject: [PATCH 2/6] indenting?? --- pyrsistent/_checked_types.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/pyrsistent/_checked_types.py b/pyrsistent/_checked_types.py index 6d3a9a0..06ea4e6 100644 --- a/pyrsistent/_checked_types.py +++ b/pyrsistent/_checked_types.py @@ -242,19 +242,19 @@ def optional(*typs): def _checked_type_create(cls, source_data, _factory_fields=None): - if isinstance(source_data, cls): - return source_data - - # Recursively apply create methods of checked types if the types of the supplied data - # does not match any of the valid types. - types = get_types(cls._checked_types) - checked_type = next((t for t in types if issubclass(t, CheckedType)), None) - if checked_type: - return cls([checked_type.create(data) - if not any(isinstance(data, t) for t in types) else data - for data in source_data]) - - return cls(source_data) + if isinstance(source_data, cls): + return source_data + + # Recursively apply create methods of checked types if the types of the supplied data + # does not match any of the valid types. + types = get_types(cls._checked_types) + checked_type = next((t for t in types if issubclass(t, CheckedType)), None) + if checked_type: + return cls([checked_type.create(data) + if not any(isinstance(data, t) for t in types) else data + for data in source_data]) + + return cls(source_data) @six.add_metaclass(_CheckedTypeMeta) class CheckedPVector(PythonPVector, CheckedType): From e6e96e51f59347fe32977377fdbabbcd13b977d1 Mon Sep 17 00:00:00 2001 From: Neil Vyas Date: Mon, 19 Feb 2018 15:58:14 -0800 Subject: [PATCH 3/6] Introducing some coverage for Enum types in seq_fields --- tests/field_test.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/tests/field_test.py b/tests/field_test.py index 870ccd9..da41d5a 100644 --- a/tests/field_test.py +++ b/tests/field_test.py @@ -1,18 +1,23 @@ -from pyrsistent import field +from pyrsistent._compat import Enum +from pyrsistent import field, pvector_field -def test_enum(): - try: - from enum import Enum - except ImportError: - # skip enum test if Enums are not available - return - class TestEnum(Enum): - x = 1 - y = 2 +class TestEnum(Enum): + x = 1 + y = 2 + +def test_enum(): f = field(type=TestEnum) assert TestEnum in f.type assert len(f.type) == 1 + + +# This is meant to exercise `_seq_field`. +def test_pvector_field_enum_type(): + f = pvector_field(TestEnum) + + assert len(f.type) == 1 + assert TestEnum is list(f.type)[0].__type__ From 6219cb28a825007e08090373f43196a748de17fd Mon Sep 17 00:00:00 2001 From: Neil Vyas Date: Mon, 19 Feb 2018 17:31:51 -0800 Subject: [PATCH 4/6] Refactoring 'field' to use the same function for user-specified-type-coercion, in order to have the same semantics across all fields. --- pyrsistent/_field_common.py | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/pyrsistent/_field_common.py b/pyrsistent/_field_common.py index 4b28733..b1e5898 100644 --- a/pyrsistent/_field_common.py +++ b/pyrsistent/_field_common.py @@ -1,18 +1,20 @@ from collections import Iterable import six -from pyrsistent._compat import Enum from pyrsistent._checked_types import ( - CheckedType, CheckedPSet, CheckedPMap, CheckedPVector, - optional as optional_type, InvariantException, get_type, wrap_invariant, - _restore_pickle, get_type) - - -def isenum(type_): - try: - return issubclass(type_, Enum) - except TypeError: - return False # type_ is not a class + CheckedPMap, + CheckedPSet, + CheckedPVector, + CheckedType, + InvariantException, + _restore_pickle, + get_type, + maybe_type_to_list, + maybe_types_to_list, +) +from pyrsistent._checked_types import optional as optional_type +from pyrsistent._checked_types import wrap_invariant +from pyrsistent._compat import Enum def set_fields(dct, bases, name): @@ -86,11 +88,15 @@ def field(type=PFIELD_NO_TYPE, invariant=PFIELD_NO_INVARIANT, initial=PFIELD_NO_ :param serializer: function that returns a serialized version of the field """ - if isinstance(type, Iterable) and not isinstance(type, six.string_types) and not isenum(type): - # Enums and strings are iterable types - types = set(type) + # NB: We have to check this predicate separately from the predicates in + # `maybe_type_to_list` et al. because this one is related to supporting the + # argspec for `field`, while those are related to supporting the valid ways + # to specify types. + # Multiple types must be passed in a tuple or a list. + if isinstance(type, (tuple, list)): + types = set(maybe_types_to_list(type)) else: - types = set([type]) + types = set(maybe_type_to_list(type)) invariant_function = wrap_invariant(invariant) if invariant != PFIELD_NO_INVARIANT and callable(invariant) else invariant field = _PField(type=types, invariant=invariant_function, initial=initial, From a001afa8e0c7e7377774e574be851ca2ff2ebb17 Mon Sep 17 00:00:00 2001 From: Neil Vyas Date: Mon, 19 Feb 2018 17:56:41 -0800 Subject: [PATCH 5/6] Allowing 'set' as a valid container for multiple fields. --- pyrsistent/_field_common.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pyrsistent/_field_common.py b/pyrsistent/_field_common.py index b1e5898..7218a29 100644 --- a/pyrsistent/_field_common.py +++ b/pyrsistent/_field_common.py @@ -92,8 +92,12 @@ def field(type=PFIELD_NO_TYPE, invariant=PFIELD_NO_INVARIANT, initial=PFIELD_NO_ # `maybe_type_to_list` et al. because this one is related to supporting the # argspec for `field`, while those are related to supporting the valid ways # to specify types. - # Multiple types must be passed in a tuple or a list. - if isinstance(type, (tuple, list)): + + # Multiple types must be passed in one of the following containers. Note + # that a type that is a subclass of one of these containers, like a + # `collections.namedtuple`, will work as expected, since we check + # `isinstance` and not `issubclass`. + if isinstance(type, (list, set, tuple)): types = set(maybe_types_to_list(type)) else: types = set(maybe_type_to_list(type)) From f05ea24cdd6f7e6122d280e11c3487b7c9257504 Mon Sep 17 00:00:00 2001 From: Neil Vyas Date: Mon, 26 Mar 2018 11:25:36 -0700 Subject: [PATCH 6/6] Refactoring to consolidate user-given-type validation, prettify names, update coverage, and leave comments as suggested. --- pyrsistent/_checked_types.py | 64 +++++++++++++++++++++--------------- pyrsistent/_field_common.py | 14 ++++---- tests/field_test.py | 4 +++ 3 files changed, 49 insertions(+), 33 deletions(-) diff --git a/pyrsistent/_checked_types.py b/pyrsistent/_checked_types.py index 06ea4e6..f5f1c13 100644 --- a/pyrsistent/_checked_types.py +++ b/pyrsistent/_checked_types.py @@ -47,48 +47,60 @@ def __str__(self): missing_fields=', '.join(self.missing_fields)) -_preserved_iterables = ( +_preserved_iterable_types = ( Enum, ) -"""For types that are themselves iterable, we would mistakenly take their -elements for the type annotation. This set defines types to 'preserve' by not -iterating over them.""" +"""Some types are themselves iterable, but we want to use the type itself and +not its members for the type specification. This defines a set of such types +that we explicitly preserve. +Note that strings are not such types because the string inputs we pass in are +values, not types. +""" -def maybe_type_to_list(t): - """Try to coerce a user-supplied type directive into a list of types.""" - # TODO FIXME: Move type validation inside this function to consolidate it. - # Preserve type annotations that are thmeselves iterable. - if isinstance(t, type) and issubclass(t, _preserved_iterables): + +def maybe_parse_user_type(t): + """Try to coerce a user-supplied type directive into a list of types. + + This function should be used in all places where a user specifies a type, + for consistency. + + The policy for what defines valid user input should be clear from the implementation. + """ + is_type = isinstance(t, type) + is_preserved = isinstance(t, type) and issubclass(t, _preserved_iterable_types) + is_string = isinstance(t, string_types) + is_iterable = isinstance(t, Iterable) + + if is_preserved: return [t] - # Preserve type annotations given as strings. - elif isinstance(t, string_types): + elif is_string: return [t] + elif is_type and not is_iterable: + return [t] + elif is_iterable: + # Recur to validate contained types as well. + ts = t + return tuple(e for t in ts for e in maybe_parse_user_type(t)) else: - if not isinstance(t, Iterable): - return [t] - else: - return list(t) - + # If this raises because `t` cannot be formatted, so be it. + raise TypeError( + 'Type specifications must be types or strings. Input: {}'.format(t) + ) -def maybe_types_to_list(ts): - res = [] - for t in ts: - res.extend(maybe_type_to_list(t)) - - return tuple(res) +def maybe_parse_many_user_types(ts): + # Just a different name to communicate that you're parsing multiple user + # inputs. `maybe_parse_user_type` handles the iterable case anyway. + return maybe_parse_user_type(ts) def _store_types(dct, bases, destination_name, source_name): - maybe_types = maybe_types_to_list([ + maybe_types = maybe_parse_many_user_types([ d[source_name] for d in ([dct] + [b.__dict__ for b in bases]) if source_name in d ]) - if not all(isinstance(t, type) or isinstance(t, string_types) for t in maybe_types): - raise TypeError('Type specifications must be types or strings') - dct[destination_name] = maybe_types diff --git a/pyrsistent/_field_common.py b/pyrsistent/_field_common.py index 7218a29..ffb3a92 100644 --- a/pyrsistent/_field_common.py +++ b/pyrsistent/_field_common.py @@ -9,8 +9,8 @@ InvariantException, _restore_pickle, get_type, - maybe_type_to_list, - maybe_types_to_list, + maybe_parse_user_type, + maybe_parse_many_user_types, ) from pyrsistent._checked_types import optional as optional_type from pyrsistent._checked_types import wrap_invariant @@ -89,18 +89,18 @@ def field(type=PFIELD_NO_TYPE, invariant=PFIELD_NO_INVARIANT, initial=PFIELD_NO_ """ # NB: We have to check this predicate separately from the predicates in - # `maybe_type_to_list` et al. because this one is related to supporting the - # argspec for `field`, while those are related to supporting the valid ways - # to specify types. + # `maybe_parse_user_type` et al. because this one is related to supporting + # the argspec for `field`, while those are related to supporting the valid + # ways to specify types. # Multiple types must be passed in one of the following containers. Note # that a type that is a subclass of one of these containers, like a # `collections.namedtuple`, will work as expected, since we check # `isinstance` and not `issubclass`. if isinstance(type, (list, set, tuple)): - types = set(maybe_types_to_list(type)) + types = set(maybe_parse_many_user_types(type)) else: - types = set(maybe_type_to_list(type)) + types = set(maybe_parse_user_type(type)) invariant_function = wrap_invariant(invariant) if invariant != PFIELD_NO_INVARIANT and callable(invariant) else invariant field = _PField(type=types, invariant=invariant_function, initial=initial, diff --git a/tests/field_test.py b/tests/field_test.py index da41d5a..cf963cf 100644 --- a/tests/field_test.py +++ b/tests/field_test.py @@ -3,6 +3,10 @@ from pyrsistent import field, pvector_field +# NB: This derives from the internal `pyrsistent._compat.Enum` in order to +# simplify coverage across python versions. Since we use +# `pyrsistent._compat.Enum` in `pyrsistent`'s implementation, it's useful to +# use it in the test coverage as well, for consistency. class TestEnum(Enum): x = 1 y = 2