New attr.cmp_using helper function

[botant]

Pull Request Check List

This is just a friendly reminder about the most common mistakes. Please make sure that you tick all boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing left to do. If your pull request is a documentation fix or a trivial typo, feel free to delete the whole thing.

• Added tests for changed code.
• New features have been added to our Hypothesis testing strategy.
• Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
  • [?] ...and used in the stub test file tests/typing_example.py.
• Updated documentation for changed code.
  • New functions/classes have to be added to docs/api.rst by hand.
  • Changes to the signature of @attr.s() have to be added by hand too.
  • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives. Find the appropriate next version in our __init__.py file.
• Documentation in .rst files is written using semantic newlines.
• Changes (and possible deprecations) have news fragments in changelog.d.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

[botant]

Hello @hynek, I've added the function cmp_using in this pull request.

It is still draft for one reason. I've used functools.total_ordering to complete the ordering functions, and I found that the functions such as _ge_from_lt are defined directly in the functools module, and because of that I can't set their __qualname__ to get well-behaved methods.

My question therefore is "how much does this matter"? If this this matters, then we need an alternative to @total_ordering.

Let me know what you think.

Thanks

[botant]

Example of numpy usage:

Python 3.8.5 (default, Jan 27 2021, 15:41:15) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.20.0 -- An enhanced Interactive Python. Type '?' for help.
PyDev console: using IPython 7.20.0
Python 3.8.5 (default, Jan 27 2021, 15:41:15) 
[GCC 9.3.0] on linux
import numpy as np
import attr
@attr.s(eq=True, order=False)
class A:
    value = attr.ib()
    
a1 = A(np.array([[1,2],[3,4]]))
a2 = A(np.array([[1,2],[3,4]]))
a1 == a2
Traceback (most recent call last):
  File "/home/antonio/.venv/attrs/lib/python3.8/site-packages/IPython/core/interactiveshell.py", line 3427, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-7-74942f9c531d>", line 1, in <module>
    a1 == a2
  File "<attrs generated eq __main__.A>", line 4, in __eq__
    return  (
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

@attr.s(eq=True, order=False)
class B:
    value = attr.ib(eq=attr.cmp_using(eq=np.array_equal))
    
<ipython-input-8-6aa555853f34>:3: SyntaxWarning: You have customized the behavior of `eq` but not of `order`.  This is probably a bug.
  value = attr.ib(eq=attr.cmp_using(eq=np.array_equal))
b1 = B(np.array([[1,2],[3,4]]))
b2 = B(np.array([[1,2],[3,4]]))
b1 == b2
Out[11]: True

[botant]

Finally, I believe we should update the following warning:

@attr.s(eq=True, order=False)
class B:
    value = attr.ib(eq=attr.cmp_using(eq=np.array_equal))
    
<ipython-input-8-6aa555853f34>:3: SyntaxWarning: You have customized the behavior of `eq` but not of `order`.  This is probably a bug.
  value = attr.ib(eq=attr.cmp_using(eq=np.array_equal))

If it is possible to tell that order=False for the entire class, then we can skip the warning altogether. If not, we can suggest setting order=False.

[hynek]

Oh god that's a lot more code than I hoped for. 😅

Firstly, I don't think the qualname stuff matters at all, but it seems like your Comparable creation is more complicated than it needs to be.

Have you considered building a dict and then using new_class() (we have a helper in _compat for Python 2) instead of patching an existing class?

Also you broke almost all builds. :D

[botant]

Got it, I'll simplify it then. :-) PS: the builds are breaking intentionally, as I committed a failing test. That is why I marked the PR as draft.

…

On Wed, 14 Apr 2021 at 12:10, Hynek Schlawack ***@***.***> wrote: Oh god that's a lot more code than I hoped for. 😅 Firstly, I don't think the qualname stuff matters at all, but it seems like your Comparable creation is more complicated than it needs to be. Have you considered building a dict and then using new_class() (we have a helper in _compat for Python 2) instead of patching an existing class? Also you broke almost all builds. :D — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#787 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABMJFPAEFEZHLO57VV6L2HTTIVZZ5ANCNFSM42X3QSBQ> .

[hynek]

cough 😇

[botant]

So... here is a version using new_class, passing tests and 100% coverage.

I even installed Python 2.7 to be sure everything worked. 😄

[hynek]

@botant thanks! would you mind removing the stray white space edits from the pyi file? Or are they on purpose?

@Julian since IIRC this is implementing an idea of yours, would you mind having a look if it’s what you had in mind?

[botant]

From __init__? I found that weird as well, but I assumed it was okay since it was formatted by black / pre-commit.

Happy to rollback if you want.

[hynek]

Yeah, __init__.pyi. I think we might be still excluding pyi files from black? I’ll need to re-investigate. Anyhow, the removed empty lines are nice, please give them back. :)

[botant]

Sure thing.

…

On Sat, Apr 24, 2021, 14:19 Hynek Schlawack ***@***.***> wrote: Yeah, __init__.pyi. I think we might be still excluding pyi files from black? I’ll need to re-investigate. Anyhow, the removed empty lines are nice, please give them back. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#787 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABMJFPHWCVBHPQJSEFQQAZTTKLANPANCNFSM42X3QSBQ> .

[Julian]

Nice! This looks great to me yeah!

[botant]

Yeah, __init__.pyi. I think we might be still excluding pyi files from black? I’ll need to re-investigate. Anyhow, the removed empty lines are nice, please give them back. :)

Revert done. Pre-commit is not picking up any formatting error after reverting the commit, so all good. I might have caused this by running black directly rather than via pre-commit.

[hynek]

Thanks! I'll write some docs and look into the warning thing!

[Julian]

Big props for sticking with this @botant.

[botant]

Thanks a lot @hynek and @Julian, it's been a pleasure to collaborate. Great experience (for me 😄).