Split cmp into eq and order (#574)

* Split cmp into eq and order Fixes #170 * Fix tests on old pypy3 versions Old as in: currently on AP. * Fix issue number and clarify newsfragment * Clarify behavior and interaction between cmp/eq/order * This sounds better * Address Julian's review comments * Missed a cmp * Test the behavior of Attribute.cmp * Make test more idiomatic * Explain assumptions * Clarify comment * Grammar * One more cmp!
python-attrs · Sep 22, 2019 · 08fcfe9 · 08fcfe9
1 parent 6e7b9f2
commit 08fcfe9
Show file tree

Hide file tree

Showing 17 changed files with 478 additions and 163 deletions.
diff --git a/README.rst b/README.rst
@@ -72,7 +72,7 @@ After *declaring* your attributes ``attrs`` gives you:
 
 - a concise and explicit overview of the class's attributes,
 - a nice human-readable ``__repr__``,
-- a complete set of comparison methods,
+- a complete set of comparison methods (equality and ordering),
 - an initializer,
 - and much more,
 

diff --git a/changelog.d/574.deprecation.rst b/changelog.d/574.deprecation.rst
@@ -0,0 +1,11 @@
+The ``cmp`` argument to ``attr.s()`` and ``attr.ib()`` is now deprecated.
+
+Please use ``eq`` to add equality methods (``__eq__`` and ``__ne__``) and ``order`` to add ordering methods (``__lt__``, ``__le__``, ``__gt__``, and ``__ge__``) instead – just like with `dataclasses <https://docs.python.org/3/library/dataclasses.html>`_.
+
+Both are effectively ``True`` by default but it's enough to set ``eq=False`` to disable both at once.
+Passing ``eq=False, order=True`` explicitly will raise a ``ValueError`` though.
+
+Since this is arguably a deeper backward-compatibility break, it will have an extended deprecation period until 2021-06-01.
+After that day, the ``cmp`` argument will be removed.
+
+``attr.Attribute`` also isn't orderable anymore.
diff --git a/conftest.py b/conftest.py
@@ -2,8 +2,6 @@
 
 import sys
 
-import pytest
-
 from hypothesis import HealthCheck, settings
 
 
@@ -15,21 +13,6 @@ def pytest_configure(config):
     settings.load_profile("patience")
 
 
-@pytest.fixture(scope="session")
-def C():
-    """
-    Return a simple but fully featured attrs class with an x and a y attribute.
-    """
-    import attr
-
-    @attr.s
-    class C(object):
-        x = attr.ib()
-        y = attr.ib()
-
-    return C
-
-
 collect_ignore = []
 if sys.version_info[:2] < (3, 6):
     collect_ignore.extend(

diff --git a/docs/api.rst b/docs/api.rst
@@ -16,7 +16,7 @@ What follows is the API explanation, if you'd like a more hands-on introduction,
 Core
 ----
 
-.. autofunction:: attr.s(these=None, repr_ns=None, repr=True, cmp=True, hash=None, init=True, slots=False, frozen=False, weakref_slot=True, str=False, auto_attribs=False, kw_only=False, cache_hash=False, auto_exc=False)
+.. autofunction:: attr.s(these=None, repr_ns=None, repr=True, cmp=None, hash=None, init=True, slots=False, frozen=False, weakref_slot=True, str=False, auto_attribs=False, kw_only=False, cache_hash=False, auto_exc=False, eq=None, order=None)
 
    .. note::
 
@@ -102,7 +102,7 @@ Core
       ... class C(object):
       ...     x = attr.ib()
       >>> attr.fields(C).x
-      Attribute(name='x', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False)
+      Attribute(name='x', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False)
 
 
 .. autofunction:: attr.make_class
@@ -174,9 +174,9 @@ Helpers
       ...     x = attr.ib()
       ...     y = attr.ib()
       >>> attr.fields(C)
-      (Attribute(name='x', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False), Attribute(name='y', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False))
+      (Attribute(name='x', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False), Attribute(name='y', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False))
       >>> attr.fields(C)[1]
-      Attribute(name='y', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False)
+      Attribute(name='y', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False)
       >>> attr.fields(C).y is attr.fields(C)[1]
       True
 
@@ -191,9 +191,9 @@ Helpers
       ...     x = attr.ib()
       ...     y = attr.ib()
       >>> attr.fields_dict(C)
-      {'x': Attribute(name='x', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False), 'y': Attribute(name='y', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False)}
+      {'x': Attribute(name='x', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False), 'y': Attribute(name='y', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False)}
       >>> attr.fields_dict(C)['y']
-      Attribute(name='y', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False)
+      Attribute(name='y', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False)
       >>> attr.fields_dict(C)['y'] is attr.fields(C).y
       True
 
@@ -288,7 +288,7 @@ See :func:`asdict` for examples.
       >>> attr.validate(i)
       Traceback (most recent call last):
          ...
-      TypeError: ("'x' must be <type 'int'> (got '1' that is a <type 'str'>).", Attribute(name='x', default=NOTHING, validator=<instance_of validator for type <type 'int'>>, repr=True, cmp=True, hash=None, init=True, type=None, kw_only=False), <type 'int'>, '1')
+      TypeError: ("'x' must be <class 'int'> (got '1' that is a <class 'str'>).", ...)
 
 
 Validators can be globally disabled if you want to run them only in development and tests but not in production because you fear their performance impact:

diff --git a/docs/examples.rst b/docs/examples.rst
@@ -534,8 +534,6 @@ The generated ``__init__`` method will have an attribute called ``__annotations_
    However it's useful for writing your own validators or serialization frameworks.
 
 
-.. _slots:
-
 Slots
 -----
 

diff --git a/docs/extending.rst b/docs/extending.rst
@@ -16,7 +16,7 @@ So it is fairly simple to build your own decorators on top of ``attrs``:
    ... @attr.s
    ... class C(object):
    ...     a = attr.ib()
-   (Attribute(name='a', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False),)
+   (Attribute(name='a', default=NOTHING, validator=None, repr=True, eq=True, order=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False),)
 
 
 .. warning::
@@ -99,10 +99,18 @@ Here are some tips for effective use of metadata:
 
     >>> MY_TYPE_METADATA = '__my_type_metadata'
     >>>
-    >>> def typed(cls, default=attr.NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, metadata={}, type=None, converter=None):
+    >>> def typed(
+    ...     cls, default=attr.NOTHING, validator=None, repr=True,
+    ...     eq=True, order=None, hash=None, init=True, metadata={},
+    ...     type=None, converter=None
+    ... ):
     ...     metadata = dict() if not metadata else metadata
     ...     metadata[MY_TYPE_METADATA] = cls
-    ...     return attr.ib(default, validator, repr, cmp, hash, init, metadata, type, converter)
+    ...     return attr.ib(
+    ...         default=default, validator=validator, repr=repr,
+    ...         eq=eq, order=order, hash=hash, init=init,
+    ...         metadata=metadata, type=type, converter=converter
+    ...     )
     >>>
     >>> @attr.s
     ... class C(object):

diff --git a/docs/hashing.rst b/docs/hashing.rst
@@ -10,7 +10,7 @@ Hash Method Generation
    Leave it at ``None`` which means that ``attrs`` will do the right thing for you, depending on the other parameters:
 
    - If you want to make objects hashable by value: use ``@attr.s(frozen=True)``.
-   - If you want hashing and comparison by object identity: use ``@attr.s(cmp=False)``
+   - If you want hashing and equality by object identity: use ``@attr.s(eq=False)``
 
    Setting ``hash`` yourself can have unexpected consequences so we recommend to tinker with it only if you know exactly what you're doing.
 
@@ -34,13 +34,13 @@ Because according to the definition_ from the official Python docs, the returned
    Python 2 on the other hand will happily let you shoot your foot off.
    Unfortunately ``attrs`` currently mimics Python 2's behavior for backward compatibility reasons if you set ``hash=False``.
 
-   The *correct way* to achieve hashing by id is to set ``@attr.s(cmp=False)``.
-   Setting ``@attr.s(hash=False)`` (that implies ``cmp=True``) is almost certainly a *bug*.
+   The *correct way* to achieve hashing by id is to set ``@attr.s(eq=False)``.
+   Setting ``@attr.s(hash=False)`` (which implies ``eq=True``) is almost certainly a *bug*.
 
    .. warning::
 
       Be careful when subclassing!
-      Setting ``cmp=False`` on class whose base class has a non-default ``__hash__`` method, will *not* make ``attrs`` remove that ``__hash__`` for you.
+      Setting ``eq=False`` on a class whose base class has a non-default ``__hash__`` method will *not* make ``attrs`` remove that ``__hash__`` for you.
 
       It is part of ``attrs``'s philosophy to only *add* to classes so you have the freedom to customize your classes as you wish.
       So if you want to *get rid* of methods, you'll have to do it by hand.
@@ -65,8 +65,9 @@ For a more thorough explanation of this topic, please refer to this blog post: `
 
 Hashing and Mutability
 ----------------------
+
 Changing any field involved in hash code computation after the first call to ``__hash__`` (typically this would be after its insertion into a hash-based collection) can result in silent bugs.
-Therefore, it is strongly recommended that hashable classes be ``frozen.``
+Therefore, it is strongly recommended that hashable classes be ``frozen``.
 Beware, however, that this is not a complete guarantee of safety:
 if a field points to an object and that object is mutated, the hash code may change, but ``frozen`` will not protect you.
 

diff --git a/src/attr/__init__.pyi b/src/attr/__init__.pyi
@@ -53,16 +53,14 @@ class Attribute(Generic[_T]):
     validator: Optional[_ValidatorType[_T]]
     repr: _ReprArgType
     cmp: bool
+    eq: bool
+    order: bool
     hash: Optional[bool]
     init: bool
     converter: Optional[_ConverterType[_T]]
     metadata: Dict[Any, Any]
     type: Optional[Type[_T]]
     kw_only: bool
-    def __lt__(self, x: Attribute[_T]) -> bool: ...
-    def __le__(self, x: Attribute[_T]) -> bool: ...
-    def __gt__(self, x: Attribute[_T]) -> bool: ...
-    def __ge__(self, x: Attribute[_T]) -> bool: ...
 
 # NOTE: We had several choices for the annotation to use for type arg:
 # 1) Type[_T]
@@ -92,14 +90,16 @@ def attrib(
     default: None = ...,
     validator: None = ...,
     repr: _ReprArgType = ...,
-    cmp: bool = ...,
+    cmp: Optional[bool] = ...,
     hash: Optional[bool] = ...,
     init: bool = ...,
     metadata: Optional[Mapping[Any, Any]] = ...,
     type: None = ...,
     converter: None = ...,
     factory: None = ...,
     kw_only: bool = ...,
+    eq: Optional[bool] = ...,
+    order: Optional[bool] = ...,
 ) -> Any: ...
 
 # This form catches an explicit None or no default and infers the type from the other arguments.
@@ -108,14 +108,16 @@ def attrib(
     default: None = ...,
     validator: Optional[_ValidatorArgType[_T]] = ...,
     repr: _ReprArgType = ...,
-    cmp: bool = ...,
+    cmp: Optional[bool] = ...,
     hash: Optional[bool] = ...,
     init: bool = ...,
     metadata: Optional[Mapping[Any, Any]] = ...,
     type: Optional[Type[_T]] = ...,
     converter: Optional[_ConverterType[_T]] = ...,
     factory: Optional[Callable[[], _T]] = ...,
     kw_only: bool = ...,
+    eq: Optional[bool] = ...,
+    order: Optional[bool] = ...,
 ) -> _T: ...
 
 # This form catches an explicit default argument.
@@ -124,14 +126,16 @@ def attrib(
     default: _T,
     validator: Optional[_ValidatorArgType[_T]] = ...,
     repr: _ReprArgType = ...,
-    cmp: bool = ...,
+    cmp: Optional[bool] = ...,
     hash: Optional[bool] = ...,
     init: bool = ...,
     metadata: Optional[Mapping[Any, Any]] = ...,
     type: Optional[Type[_T]] = ...,
     converter: Optional[_ConverterType[_T]] = ...,
     factory: Optional[Callable[[], _T]] = ...,
     kw_only: bool = ...,
+    eq: Optional[bool] = ...,
+    order: Optional[bool] = ...,
 ) -> _T: ...
 
 # This form covers type=non-Type: e.g. forward references (str), Any
@@ -140,22 +144,24 @@ def attrib(
     default: Optional[_T] = ...,
     validator: Optional[_ValidatorArgType[_T]] = ...,
     repr: _ReprArgType = ...,
-    cmp: bool = ...,
+    cmp: Optional[bool] = ...,
     hash: Optional[bool] = ...,
     init: bool = ...,
     metadata: Optional[Mapping[Any, Any]] = ...,
     type: object = ...,
     converter: Optional[_ConverterType[_T]] = ...,
     factory: Optional[Callable[[], _T]] = ...,
     kw_only: bool = ...,
+    eq: Optional[bool] = ...,
+    order: Optional[bool] = ...,
 ) -> Any: ...
 @overload
 def attrs(
     maybe_cls: _C,
     these: Optional[Dict[str, Any]] = ...,
     repr_ns: Optional[str] = ...,
     repr: bool = ...,
-    cmp: bool = ...,
+    cmp: Optional[bool] = ...,
     hash: Optional[bool] = ...,
     init: bool = ...,
     slots: bool = ...,
@@ -166,14 +172,16 @@ def attrs(
     kw_only: bool = ...,
     cache_hash: bool = ...,
     auto_exc: bool = ...,
+    eq: Optional[bool] = ...,
+    order: Optional[bool] = ...,
 ) -> _C: ...
 @overload
 def attrs(
     maybe_cls: None = ...,
     these: Optional[Dict[str, Any]] = ...,
     repr_ns: Optional[str] = ...,
     repr: bool = ...,
-    cmp: bool = ...,
+    cmp: Optional[bool] = ...,
     hash: Optional[bool] = ...,
     init: bool = ...,
     slots: bool = ...,
@@ -184,6 +192,8 @@ def attrs(
     kw_only: bool = ...,
     cache_hash: bool = ...,
     auto_exc: bool = ...,
+    eq: Optional[bool] = ...,
+    order: Optional[bool] = ...,
 ) -> Callable[[_C], _C]: ...
 
 # TODO: add support for returning NamedTuple from the mypy plugin
@@ -202,7 +212,7 @@ def make_class(
     bases: Tuple[type, ...] = ...,
     repr_ns: Optional[str] = ...,
     repr: bool = ...,
-    cmp: bool = ...,
+    cmp: Optional[bool] = ...,
     hash: Optional[bool] = ...,
     init: bool = ...,
     slots: bool = ...,
@@ -213,6 +223,8 @@ def make_class(
     kw_only: bool = ...,
     cache_hash: bool = ...,
     auto_exc: bool = ...,
+    eq: Optional[bool] = ...,
+    order: Optional[bool] = ...,
 ) -> type: ...
 
 # _funcs --