From f18162bd94d0ff88886429b90bf52af4aef0425d Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Fri, 13 Nov 2020 13:54:41 -0800 Subject: [PATCH 1/4] Clarify that Set._from_iterable is not required to be a classmethod. In the implementation, `_from_iterable` is only used via `self`, and requiring `_from_iterable` to be a classmethod makes it of little value to subclasses with different constructor signatures. --- Doc/library/collections.abc.rst | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Doc/library/collections.abc.rst b/Doc/library/collections.abc.rst index db0e25bb0772eb..117cd19c3c7123 100644 --- a/Doc/library/collections.abc.rst +++ b/Doc/library/collections.abc.rst @@ -287,12 +287,11 @@ Notes on using :class:`Set` and :class:`MutableSet` as a mixin: Since some set operations create new sets, the default mixin methods need a way to create new instances from an iterable. The class constructor is assumed to have a signature in the form ``ClassName(iterable)``. - That assumption is factored-out to an internal classmethod called + That assumption is factored-out to an internal method called :meth:`_from_iterable` which calls ``cls(iterable)`` to produce a new set. If the :class:`Set` mixin is being used in a class with a different constructor signature, you will need to override :meth:`_from_iterable` - with a classmethod that can construct new instances from - an iterable argument. + to construct new instances from an iterable argument. (2) To override the comparisons (presumably for speed, as the From f1057d4fbc47835ba704a0dc7c2ce85cf2bedae2 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 16 Nov 2020 11:37:34 -0800 Subject: [PATCH 2/4] Address review feedback. * Restore mention default impl is class doc * Rephrase to say impl can be class or regular method * Add tests to verify instance method _from_iterable works --- Doc/library/collections.abc.rst | 5 +-- Lib/test/test_collections.py | 59 ++++++++++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/Doc/library/collections.abc.rst b/Doc/library/collections.abc.rst index 117cd19c3c7123..dd8f40662f287b 100644 --- a/Doc/library/collections.abc.rst +++ b/Doc/library/collections.abc.rst @@ -287,11 +287,12 @@ Notes on using :class:`Set` and :class:`MutableSet` as a mixin: Since some set operations create new sets, the default mixin methods need a way to create new instances from an iterable. The class constructor is assumed to have a signature in the form ``ClassName(iterable)``. - That assumption is factored-out to an internal method called + That assumption is factored-out to an internal classmethod method called :meth:`_from_iterable` which calls ``cls(iterable)`` to produce a new set. If the :class:`Set` mixin is being used in a class with a different constructor signature, you will need to override :meth:`_from_iterable` - to construct new instances from an iterable argument. + with a classmethod or regular method that can construct new instances from + an iterable argument. (2) To override the comparisons (presumably for speed, as the diff --git a/Lib/test/test_collections.py b/Lib/test/test_collections.py index 7c7f8655b0fbdd..adc1e509124ccf 100644 --- a/Lib/test/test_collections.py +++ b/Lib/test/test_collections.py @@ -1559,8 +1559,65 @@ def assertSameSet(self, s1, s2): # coerce both to a real set then check equality self.assertSetEqual(set(s1), set(s2)) + def test_Set_from_iterable(self): + """Verify _from_iterable overriden to an instance method works.""" + class SetUsingInstanceFromIterable(MutableSet): + def __init__(self, values, created_by): + if not created_by: + raise ValueError(f'created_by must be specified') + self.created_by = created_by + self._values = set(values) + + def _from_iterable(self, values): + return type(self)(values, 'from_iterable') + + def __contains__(self, value): + return value in self._values + + def __iter__(self): + yield from self._values + + def __len__(self): + return len(self._values) + + def add(self, value): + self._values.add(value) + + def discard(self, value): + self._values.discard(value) + + impl = SetUsingInstanceFromIterable([1, 2, 3], 'test') + + actual = impl - {1} + self.assertIsInstance(actual, SetUsingInstanceFromIterable) + self.assertEqual('from_iterable', actual.created_by) + self.assertEqual({2, 3}, actual) + + actual = impl | {4} + self.assertIsInstance(actual, SetUsingInstanceFromIterable) + self.assertEqual('from_iterable', actual.created_by) + self.assertEqual({1, 2, 3, 4}, actual) + + actual = impl & {2} + self.assertIsInstance(actual, SetUsingInstanceFromIterable) + self.assertEqual('from_iterable', actual.created_by) + self.assertEqual({2}, actual) + + actual = impl ^ {3, 4} + self.assertIsInstance(actual, SetUsingInstanceFromIterable) + self.assertEqual('from_iterable', actual.created_by) + self.assertEqual({1, 2, 4}, actual) + + # NOTE: ixor'ing with a list is important here: internally, __ixor__ + # only calls _from_iterable if the other value isn't already a Set. + impl ^= [3, 4] + self.assertIsInstance(impl, SetUsingInstanceFromIterable) + self.assertEqual('test', impl.created_by) + self.assertEqual({1, 2, 4}, impl) + + def test_Set_interoperability_with_real_sets(self): - # Issue: 8743 + # Issue: 8743 class ListSet(Set): def __init__(self, elements=()): self.data = [] From 0a373306c75db27d024c74666291fbcccadb5436 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 16 Nov 2020 19:43:40 -0800 Subject: [PATCH 3/4] Remove/fix some extra/extraneous whitespace --- Lib/test/test_collections.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/test/test_collections.py b/Lib/test/test_collections.py index adc1e509124ccf..150c2a1c0e3498 100644 --- a/Lib/test/test_collections.py +++ b/Lib/test/test_collections.py @@ -1615,9 +1615,8 @@ def discard(self, value): self.assertEqual('test', impl.created_by) self.assertEqual({1, 2, 4}, impl) - def test_Set_interoperability_with_real_sets(self): - # Issue: 8743 + # Issue: 8743 class ListSet(Set): def __init__(self, elements=()): self.data = [] From 97b9ddf907976941da52e515e5ac3afb6473087e Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 16 Nov 2020 19:45:21 -0800 Subject: [PATCH 4/4] Remove extraneous "method" word --- Doc/library/collections.abc.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/library/collections.abc.rst b/Doc/library/collections.abc.rst index dd8f40662f287b..2345e78a17e4f5 100644 --- a/Doc/library/collections.abc.rst +++ b/Doc/library/collections.abc.rst @@ -287,7 +287,7 @@ Notes on using :class:`Set` and :class:`MutableSet` as a mixin: Since some set operations create new sets, the default mixin methods need a way to create new instances from an iterable. The class constructor is assumed to have a signature in the form ``ClassName(iterable)``. - That assumption is factored-out to an internal classmethod method called + That assumption is factored-out to an internal classmethod called :meth:`_from_iterable` which calls ``cls(iterable)`` to produce a new set. If the :class:`Set` mixin is being used in a class with a different constructor signature, you will need to override :meth:`_from_iterable`