typeshed icon indicating copy to clipboard operation
typeshed copied to clipboard

Remove all `__nonzero__` methods

Open sobolevn opened this issue 2 years ago • 12 comments

I think that it is almost time (maybe after new mypy release) to remove __nonzero__ methods from typeshed.

Why?

  • python2 support is dropped from mypy
  • I removed __nonzero__ support from mypy in https://github.com/python/mypy/pull/13277

I've also removed all __nonzero__ methods from these libs:

  • https://github.com/psycopg/psycopg2/pull/1484
  • https://github.com/python-babel/babel/pull/896
  • https://github.com/Pylons/waitress/pull/384
  • https://github.com/psf/requests/pull/6207
  • https://github.com/sqlalchemy/sqlalchemy/pull/8308

Some libs, however, support both python2 and python3 (like python-dateutil).

sobolevn avatar Jul 30 '22 12:07 sobolevn

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

github-actions[bot] avatar Jul 30 '22 12:07 github-actions[bot]

Typeshed no longer supports Python 2 (even if a third-party library supports Python 2, our stubs for that library don't), so I'm fully in support of this! I don't think we need to wait for the next mypy release.

One thing we should do, however, is add __nonzero__ (and probably all other Python-2 magic methods) to this list in stubtest: https://github.com/python/mypy/blob/0d7424c9f80e5695042f21e9d2ea44661d0d6a8a/mypy/stubtest.py#L1130. Otherwise, stubtest will complain about these methods being missing from the stub (unless stubtest is running with --ignore-missing-stub, which is the setting we use for most of our third-party libraries).

AlexWaygood avatar Jul 30 '22 12:07 AlexWaygood

I'm not sure we should do this. You can still create a __nonzero__ method in Python 3; it just doesn't do anything magical. We should remove them only when they do not exist at runtime.

JelleZijlstra avatar Jul 30 '22 14:07 JelleZijlstra

@JelleZijlstra

You can still create a __nonzero__ method in Python 3; it just doesn't do anything magical.

Yes, but in all cases these methods are just copies of __bool__. They are not "something special". I've also removed almost all of them from runtime as well (except python-dateutil, invoke, and tqdm because they do still support python2).

sobolevn avatar Jul 30 '22 14:07 sobolevn

I think @JelleZijlstra has a good point, actually; I've changed my mind. Most of the __nonzero__ methods in typeshed are hangovers from Python 2, but what if some third-party code, that's potentially never supported Python 2, has a method that happens to be called __nonzero__ and means something special in the context of their third-party library or application?

We shouldn't necessarily remove all __nonzero__ methods just because they used to mean something special in the context of Python 2, and now no longer have that special meaning. They might still have other meanings in some circumstances, even if they no longer count as "magic" methods.

AlexWaygood avatar Jul 30 '22 14:07 AlexWaygood

They might still have other meanings in some circumstances, even if they no longer count as "magic" methods.

But they don't in the cases of this PR 🙂 Check out my PRs I've listed in https://github.com/python/typeshed/pull/8443#issue-1323011957 And ones that I've not covered yet:

  • https://github.com/dateutil/dateutil/blob/0cc33ad6336a68e01c777f8f580c9df219a1c17f/src/dateutil/relativedelta.py#L493
  • https://github.com/pyinvoke/invoke/blob/3177b1b1d56a36cf7f34b54b3a7d4dea692f6ab9/invoke/collection.py#L141
  • https://github.com/tqdm/tqdm/blob/87d253f65621884c9a4020fecabc7824029e2358/tqdm/std.py#L1123

sobolevn avatar Jul 30 '22 14:07 sobolevn

But they don't in the cases of this PR 🙂

Sure -- maybe we should go ahead with this PR, then (after the runtime PRs have been merged), but not go ahead with the stubtest change? 🙂

AlexWaygood avatar Jul 30 '22 14:07 AlexWaygood

but not go ahead with the stubtest change?

I can barely think of any reasonable case where __nonzero__ means somethings different. And I think that magic methods are meant to be reserved for python-level magic, not something custom.

So, in my opinion ignoring legacy is a good thing. But, I am open for other opinions (especially with real-life use-cases).

sobolevn avatar Jul 30 '22 16:07 sobolevn

And I think that magic methods are meant to be reserved for python-level magic, not something custom.

That's how they're meant to be used, for sure, but third-party libraries define their own dunder methods relatively frequently anyway -- e.g. rich has the __rich_repr__ method.

AlexWaygood avatar Jul 30 '22 17:07 AlexWaygood

I still don't see a reason for removing __nonzero__ methods from typeshed until after they have been removed in released versions of the libraries we have stubs for. Our job is to report what exists in those libraries, not what should exist.

JelleZijlstra avatar Jul 30 '22 17:07 JelleZijlstra

Agreed, let's wait a bit 👍

sobolevn avatar Jul 30 '22 19:07 sobolevn

I agree with @JelleZijlstra. Let's do whatever the stubbed library does. I'm absolutely in favor of removing __nonzero__ in the libraries themselves, though, if they dropped Python 2 support.

srittau avatar Jul 31 '22 13:07 srittau

We'd rather not make these changes until the various runtime libraries have cut releases which have the __nonzero__ methods removed. And when there are releases with the __nonzero__ methods removed, we'll have to make these changes as part of the version bump, rather than as a standalone PR. So, I'm going to close this for now :)

AlexWaygood avatar Sep 28 '22 11:09 AlexWaygood