Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-41974: Remove complex.__float__, complex.__floordiv__, etc #22593

Merged

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Oct 8, 2020

Remove complex special methods __int__, __float__, __floordiv__,
__mod__ and __divmod__ which always raised a TypeError.

https://bugs.python.org/issue41974

Remove complex special methods __int__, __float__, __floordiv__,
__mod__ and __divmod__ which always raised a TypeError.
Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and 👍 for getting rid of these.

My only doubt is whether the phrase "real number" will make sense to those reading the error messages. It may be less confusing to leave it as "number", even though that's less precise.

@serhiy-storchaka
Copy link
Member Author

serhiy-storchaka commented Oct 8, 2020

It was noticed by Guido that error message "float() argument must be a string or a number, not 'complex'" would look confusing and he proposed to use "real number".
https://mail.python.org/archives/list/python-ideas@python.org/message/KT4JNNL6NL4L77AAQBK67GQH7U5NJ5BP/

I agreed.

@mdickinson
Copy link
Member

Fair enough. The "real number" term is at least easily searchable.

with self.assertRaises(ZeroDivisionError):
1.0 / (0+0j)
with self.assertRaises(ZeroDivisionError):
1 / (0+0j)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like these 5 tests are copy/pasted in multiple test methods. Would it be possible to use a global list and write a test using this test? Something like:

for a, b in ZERO_DIVISION:
    with self.subTest(a=a, b=b):
        with self.assertRaises(ZeroDivisionError):
            a / b

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if (nbr == NULL || (nbr->nb_float == NULL && nbr->nb_index == NULL)) {
if (nbr == NULL ||
(nbr->nb_float == NULL && nbr->nb_index == NULL && !PyComplex_Check(r)))
{
PyErr_Format(PyExc_TypeError,
"complex() first argument must be a string or a number, "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you change the float error, would it make sense to write "a complex number" here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be a real number (int, float, Fraction, Decimal, etc). I think that requiring the first argument of the complex constructor to be a complex number would be confusing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An "integer" number is also a real number, and a real number is also a complex number, no? I'm thinking at "real" and "complex" such as in the numbers tower. But it's just a remark, you can leave it as it is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In mathematics it is true. But int and float are not subclasses of complex in Python.

Also, requiring an argument of the complex constructor to be a complex number would be very confusing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In mathematics it is true. But int and float are not subclasses of complex in Python.

Oh, I didn't look at that and you're right :-)

@@ -99,20 +99,48 @@ def test_truediv(self):
self.check_div(complex(random(), random()),
complex(random(), random()))

self.assertRaises(ZeroDivisionError, complex.__truediv__, 1+1j, 0+0j)
self.assertRaises(OverflowError, pow, 1e200+1j, 1e200+1j)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check done in another test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, no. I removed this test because it does not have relation to division (and the above test was duplicated).

Moved it into test_pow.

1 / (0+0j)
for a, b in ZERO_DIVISION:
with self.assertRaises(ZeroDivisionError):
a / b
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it's more readable like this!

@@ -231,6 +215,7 @@ def test_pow(self):
self.assertAlmostEqual(pow(1j, -1), 1/1j)
self.assertAlmostEqual(pow(1j, 200), 1)
self.assertRaises(ValueError, pow, 1+1j, 1+1j, 1+1j)
self.assertRaises(OverflowError, pow, 1e200+1j, 1e200+1j)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, the best better fits into test_pow(), thanks!

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I just have a minor remark on the NEWS entry.

* Removed special methods ``__int__``, ``__float__``, ``__floordiv__``,
``__mod__``, ``__divmod__``, ``__rfloordiv__``, ``__rmod__`` and
``__rdivmod__`` of the :class:`complex` class. They always raised
a :exc:`TypeError`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear if it changes the behavior. Maybe explain that these operations still exist and still raise TypeError?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed this comment. Could you propose new wording? Or maybe you want to make the documentation PR yourself?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter. Leave it as it is :-)

@serhiy-storchaka serhiy-storchaka merged commit e2ec0b2 into python:master Oct 9, 2020
@serhiy-storchaka serhiy-storchaka deleted the complex-fake-dunders branch October 9, 2020 11:14
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
…nGH-22593)

Remove complex special methods __int__, __float__, __floordiv__,
__mod__, __divmod__, __rfloordiv__, __rmod__ and __rdivmod__
which always raised a TypeError.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants