bpo-37086: fixed time.sleep error message #13768

Windsooon · 2019-06-03T03:43:21Z

https://bugs.python.org/issue37086

Windsooon · 2019-06-03T03:43:44Z

I'm not sure should I add news or not.

ericvsmith

A few nits, but otherwise looks good.

I don't think you need a news entry.

ericvsmith · 2019-06-03T22:22:11Z

Lib/test/test_time.py

+                TypeError, 'an integer or a float is required \(got type str\)') as te:
+            time.sleep('foo')
+
+


It looks like all of the other tests have just one blank line between them. Please delete one of the lines.

ericvsmith · 2019-06-03T22:22:53Z

Lib/test/test_time.py

@@ -163,6 +163,12 @@ def test_sleep(self):
        self.assertRaises(ValueError, time.sleep, -1)
        time.sleep(1.2)

+    def test_sleep_invalid(self):
+        with self.assertRaisesRegex(
+                TypeError, 'an integer or a float is required \(got type str\)') as te:


You don't need the as te part of this expression.

ericvsmith · 2019-06-03T22:25:02Z

Lib/test/test_time.py

@@ -163,6 +163,12 @@ def test_sleep(self):
        self.assertRaises(ValueError, time.sleep, -1)
        time.sleep(1.2)

+    def test_sleep_invalid(self):
+        with self.assertRaisesRegex(
+                TypeError, 'an integer or a float is required \(got type str\)') as te:


You want this to be a raw (r'') string, in order to escape the parens for the regex engine.

Yes. I think you are right. It came to me earlier but I forgot to delete it. I updated the PR, thanks.

bedevere-bot · 2019-06-03T22:25:29Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Windsooon · 2019-06-04T01:00:32Z

Thank you for review my code. I updated the PR based on your review.

ericvsmith

Sorry for the one last nit. Once you make this one and the tests pass, I'll commit it.

ericvsmith · 2019-06-04T01:13:15Z

Objects/longobject.c

@@ -207,7 +207,7 @@ _PyLong_FromNbIndexOrNbInt(PyObject *integral)
    nb = Py_TYPE(integral)->tp_as_number;
    if (nb == NULL || (nb->nb_index == NULL && nb->nb_int == NULL)) {
        PyErr_Format(PyExc_TypeError,
-                     "an integer is required (got type %.200s)",
+                     "an integer or a float is required (got type %.200s)",


I know this is trivial, but it seems more consistent with existing code to say "an integer or float is required" (that is, drop the "a"). Would you mind making that change, too? I apologize for not catching that earlier.

ericvsmith · 2019-06-04T01:17:49Z

It looks like test_datetime and test_socket are failing because they check the exception text. They'll need to be fixed, too.

Windsooon · 2019-06-04T03:28:37Z

@ericvsmith I have some questions

Should we use an integer or float is required. or an integer or float is required. (got type %s), ob->ob_name here, I found other tests didn't use a dynamic string in the PyErr_SetString function.
Should I change Invalid value NaN (not a number) to Not a float number in invalid value in this commit?

ericvsmith · 2019-06-04T09:17:25Z

@Windsooon:

If you're talking about the tests, I'd just look for an integer or float is required. But for the error message, I'd include the found type. It's very helpful in debugging.
I wouldn't change that as part of this PR. If it needs doing, I'd create a separate issue and PR.

jdemeyer

Won't this remove support for objects with __int__? See also bpo-35707.

jdemeyer · 2019-06-07T09:18:27Z

Indeed, this breaks

>>> import time, decimal; time.sleep(decimal.Decimal(1))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: an integer or float is required.

Windsooon · 2019-06-07T09:22:04Z

Thank you :D, I will update the pr as well as the tests in a few days.

ericvsmith · 2019-06-07T09:54:47Z

I think the earlier version of the patch (which didn't move the tests around) was a better design.

jdemeyer · 2019-06-07T10:58:58Z

I think it's better to merge #11636 first since that will change the structure of this code anyway and it has all the tests that you need.

iritkatriel · 2020-09-17T11:13:26Z

Python/pytime.c

@@ -401,7 +401,11 @@ static int
 _PyTime_FromObject(_PyTime_t *t, PyObject *obj, _PyTime_round_t round,
                   long unit_to_ns)
 {
-    if (PyFloat_Check(obj)) {
+    if (!PyFloat_Check(obj) && !PyLong_Check(obj)) {


I don't think this is correct. In the else case below it calls PyLong_AsLongLong, which can handle values which are not PyLongs as long as they can be converted to a PyLong via _PyNumber_Index.

If this change didn't break any unit tests, then a test is probably missing.

ah, I see you've commented on that. Sorry for the noise.

bpo-37086: fixed time.sleep error message

6c633dc

the-knights-who-say-ni added the CLA signed label Jun 3, 2019

bedevere-bot added the awaiting review label Jun 3, 2019

ericvsmith requested changes Jun 3, 2019

View reviewed changes

bedevere-bot removed the awaiting review label Jun 3, 2019

bedevere-bot added the awaiting changes label Jun 3, 2019

make the string as raw string, clean code

c8f1958

ericvsmith added the skip news label Jun 4, 2019

ericvsmith requested changes Jun 4, 2019

View reviewed changes

Windsooon added 2 commits June 4, 2019 09:24

remote 'a' before 'float is required'

54cd9c8

fixed time.sleep error message

4297234

Windsooon force-pushed the bpo-37086 branch from 993f083 to 4297234 Compare June 4, 2019 03:22

jdemeyer requested changes Jun 4, 2019

View reviewed changes

iritkatriel reviewed Sep 17, 2020

View reviewed changes

JustinFay mannequin mentioned this pull request Apr 10, 2022

time.sleep error message misleading #81267

Closed

ezio-melotti removed the CLA signed label Jul 13, 2022

erlend-aasland marked this pull request as draft February 9, 2024 11:41

bedevere-app bot removed the awaiting changes label Feb 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-37086: fixed time.sleep error message #13768

bpo-37086: fixed time.sleep error message #13768

Windsooon commented Jun 3, 2019 •

edited by bedevere-bot

Loading

Windsooon commented Jun 3, 2019

ericvsmith left a comment

ericvsmith Jun 3, 2019

ericvsmith Jun 3, 2019

ericvsmith Jun 3, 2019

Windsooon Jun 4, 2019

bedevere-bot commented Jun 3, 2019

Windsooon commented Jun 4, 2019

ericvsmith left a comment

ericvsmith Jun 4, 2019

ericvsmith commented Jun 4, 2019

Windsooon commented Jun 4, 2019 •

edited

Loading

ericvsmith commented Jun 4, 2019

jdemeyer left a comment •

edited

Loading

jdemeyer commented Jun 7, 2019

Windsooon commented Jun 7, 2019 via email

ericvsmith commented Jun 7, 2019

jdemeyer commented Jun 7, 2019

iritkatriel Sep 17, 2020 •

edited

Loading

iritkatriel Sep 17, 2020

		TypeError, 'an integer or a float is required \(got type str\)') as te:
		time.sleep('foo')

bpo-37086: fixed time.sleep error message #13768

Are you sure you want to change the base?

bpo-37086: fixed time.sleep error message #13768

Conversation

Windsooon commented Jun 3, 2019 • edited by bedevere-bot Loading

Windsooon commented Jun 3, 2019

ericvsmith left a comment

Choose a reason for hiding this comment

ericvsmith Jun 3, 2019

Choose a reason for hiding this comment

ericvsmith Jun 3, 2019

Choose a reason for hiding this comment

ericvsmith Jun 3, 2019

Choose a reason for hiding this comment

Windsooon Jun 4, 2019

Choose a reason for hiding this comment

bedevere-bot commented Jun 3, 2019

Windsooon commented Jun 4, 2019

ericvsmith left a comment

Choose a reason for hiding this comment

ericvsmith Jun 4, 2019

Choose a reason for hiding this comment

ericvsmith commented Jun 4, 2019

Windsooon commented Jun 4, 2019 • edited Loading

ericvsmith commented Jun 4, 2019

jdemeyer left a comment • edited Loading

Choose a reason for hiding this comment

jdemeyer commented Jun 7, 2019

Windsooon commented Jun 7, 2019 via email

ericvsmith commented Jun 7, 2019

jdemeyer commented Jun 7, 2019

iritkatriel Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

iritkatriel Sep 17, 2020

Choose a reason for hiding this comment

Windsooon commented Jun 3, 2019 •

edited by bedevere-bot

Loading

Windsooon commented Jun 4, 2019 •

edited

Loading

jdemeyer left a comment •

edited

Loading

iritkatriel Sep 17, 2020 •

edited

Loading