bpo-30302 Make timedelta.repr more informative. #1493

musically-ut · 2017-05-07T19:59:31Z

Currently, the default implementation of datetime.datetime.repr (the default output string produced at the console/IPython) gives a rather cryptic output:

from datetime import datetime as D
D.fromtimestamp(1390953543.1) - D.fromtimestamp(1121871596)
# datetime.timedelta(3114, 28747, 100000)

For the uninitiated, it is not obvious that the numeric values here are days, seconds and microsecond respectively.

Would there be any pushback against changing this to:

# datetime.timedelta(days=3114, seconds=28747, microseconds=100000)

?

https://bugs.python.org/issue30302

the-knights-who-say-ni · 2017-05-07T19:59:33Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

mention-bot · 2017-05-07T19:59:35Z

@musically-ut, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tim-one, @abalkin and @serhiy-storchaka to be potential reviewers.

vadmium · 2017-05-07T23:51:44Z

As well as tests, the documentation needs reviewing. It actually specifies the repr return value.

musically-ut · 2017-05-07T23:55:40Z

@vadmium Good point. I'm assuming that the documentation is not limited to only the doc strings (actually, I couldn't find an instance there on a cursory look).

Is there an easy way to check that I have caught all cases in the extended documentation?

musically-ut · 2017-05-08T00:08:05Z

@vadmium I see what you meant by:

It actually specifies the repr return value.

here: https://github.com/python/cpython/blob/3.6/Doc/library/datetime.rst

So we'll have to consider the risk of someone's code breaking because it depends on parsing the repr representation of timedelta?

Update: Related mail thread addresses the question somewhat: https://marc.info/?l=python-dev&m=145065097122111&w=2

vstinner · 2017-06-27T14:26:30Z

Lib/datetime.py

-        return "%s.%s(%d)" % (self.__class__.__module__,
+            if arg_str != "":
+                arg_str += ", "
+            arg_str += "microseconds=%d" % self._microseconds


Concatenate strings using += is inefficient. Moreover, you have to handle comma manually. Please rewrite this code using a list (args = [] ... args.append(...)), and then join it using: ', '.join(args).

Thanks! Will do that.

vstinner · 2017-06-27T14:27:04Z

Lib/test/datetimetester.py

+                         "%s(days=1, microseconds=100)" % name)
+        self.assertEqual(repr(self.theclass(seconds=1, microseconds=100)),
+                         "%s(seconds=1, microseconds=100)" % name)
+        # self.assertEqual(repr(self.theclass(seconds=-60)),


Why are these tests commented? Either enable them or remove them.

Martin wanted the handling of negative attributes to change while I am of the opinion that they shouldn't. So we may end up discussing these tests later.

However, I'll remove them from the PR presently.

vstinner · 2017-06-27T14:28:17Z

Modules/_datetimemodule.c

-                                    GET_TD_SECONDS(self));
-
-    return PyUnicode_FromFormat("%s(%d)",
+    char days[64] = "", seconds[64] = "", microseconds[64] = "";


Hum, the >= ""< part is really strange (remove it?).

Moreover, why 64 bytes? Microseconds cannot be larger than 999 999, so 6 digits + 1 null byte, no?

I got the 64 bytes number from here, which could also be 6 digits + 1. However, in light of your next comment, we may not end up needing this at all.

vstinner · 2017-06-27T14:29:07Z

Modules/_datetimemodule.c

+        }
+    }
+
+    return PyUnicode_FromFormat("%s(%s%s%s)",
                                Py_TYPE(self)->tp_name,


Hum, no module nor qualified name here?

tp_name contains the fully qualified name.

vstinner · 2017-06-27T14:31:20Z

Modules/_datetimemodule.c

+            sprintf(seconds, "seconds=%d", GET_TD_SECONDS(self));
+        } else {
+            sprintf(seconds, ", seconds=%d", GET_TD_SECONDS(self));
+        }


Hum, the code may be simpler if you use a PyList and ", ".join(), as proposed for the Python code. You can use PyUnicode_FromString("seconds=%d", GET_TD_SECONDS(self)) to build an argument. It would avoid to have to compute a buffer size (and taking the risk of a buffer overflow).

I've implemented a version using PyList and PyUnicode. I don't think I'm leaking any memory there, but since this is the first time I'm touching PyObjects, I would really appreciate a second opinion on it.

serhiy-storchaka

The result of C API function calls always should be checked for error.

There are memory leaks.

serhiy-storchaka · 2017-06-27T17:03:44Z

Modules/_datetimemodule.c

                                Py_TYPE(self)->tp_name,
-                                GET_TD_DAYS(self));
+                                args_string);


args_string is leaked.

serhiy-storchaka · 2017-06-27T17:04:13Z

Modules/_datetimemodule.c

+        PyList_Append(args, PyUnicode_FromFormat("microseconds=%d", GET_TD_MICROSECONDS(self)));
+    }
+
+    PyObject *args_string = PyUnicode_Join(PyUnicode_FromString(", "), args);


The result of PyUnicode_FromString(", ") is leaked.

serhiy-storchaka · 2017-06-27T17:04:59Z

Modules/_datetimemodule.c

+    PyObject *args = PyList_New(0);
+
+    if (GET_TD_DAYS(self) != 0) {
+        PyList_Append(args, PyUnicode_FromFormat("days=%d", GET_TD_DAYS(self)));


PyUnicode_FromFormat() and PyList_Append() can fail.

The result of PyUnicode_FromFormat() is leaked.

serhiy-storchaka · 2017-06-27T17:05:33Z

Modules/_datetimemodule.c

-                                    GET_TD_SECONDS(self));
-
-    return PyUnicode_FromFormat("%s(%d)",
+    PyObject *args = PyList_New(0);


PyList_New(0) can fail.

serhiy-storchaka · 2017-06-27T17:07:41Z

Lib/datetime.py

-        return "%s.%s(%d)" % (self.__class__.__module__,
+            args.append("microseconds=%d" % self._microseconds)
+
+        arg_str = ', '.join(args)


Just inline arg_str. This is simple expression.

Done. Thanks!

serhiy-storchaka · 2017-06-27T17:10:07Z

Lib/test/test_datetime.py

@@ -20,7 +20,7 @@
 # XXX(gb) First run all the _Pure tests, then all the _Fast tests.  You might
 # not believe this, but in spite of all the sys.modules trickery running a _Pure
 # test last will leave a mix of pure and native datetime stuff lying around.
-test_classes = []
+all_test_classes = []


Why this is changed?

Because the tests were only running for the C implementation, not for the Python
implementation. This was because test_classes was being overwritten inside the for loop. With this change, the tests run twice: once for the pure Python module and once for the C extension.

Please fix the loop rather than renaming this variable.

Ah, sorry, I didn't recognize that it was a comment on this review, Github didn't allow me to reply to this from where I actually saw it.

So, yes, the loop is correct, the only thing it was doing incorrectly was overwriting the test_classes variable when it moved from '_Pure' classes to '_Fast' classes. It does need to name them differently (I'm not sure why the names of the classes don't actually change in the verbose output, that seems to take me down the test_suite rabbit hole to figure out whether __name__ is the correct attribute to change or not). However, the behavior of the tests is correct now, i.e. it tests both implementations in sequence.

I'm investigating whether there is a simple fix to make the correct version of classes to show up in the tests.

Because the tests were only running for the C implementation, not for the Python
implementation.

This looks like a bug. Please open a new issue for it. This should be fixed in all versions.

Good point. I'll check which versions are affected (i.e. the branches on the cpython repository, right?) and open another bug. What should I do with this fix, though?

If I let it remain here, it may potentially lead to a merge conflict?

I've created http://bugs.python.org/issue30822

After merging the PR that fixes the tests this PR should be merged with master. Merge conflicts that are not resolved automatically should be resolved manually.

serhiy-storchaka · 2017-06-27T18:00:13Z

Modules/_datetimemodule.c

+            Py_XDECREF(days);
+            return NULL;
+        }
+    }


You can decref days here. This will simplify the cases for seconds and microseconds.

serhiy-storchaka · 2017-06-27T18:01:07Z

Modules/_datetimemodule.c

+
+        if (PyList_Append(args, days) < 0) {
+            Py_DECREF(args);
+            Py_XDECREF(days);


Just Py_DECREF. days is not NULL.

Or you can merge two ifs:

if (days == NULL || PyList_Append(args, days) < 0) { Py_DECREF(args); Py_XDECREF(days); return NULL; }

serhiy-storchaka · 2017-06-27T18:05:24Z

Modules/_datetimemodule.c

+    }
+
+    PyObject *args_string = PyUnicode_Join(sep, args);
+


You can decref sep and args just here.

musically-ut · 2017-06-27T20:39:09Z

@serhiy-storchaka Thanks for the suggestions! The code looks much cleaner and more concise now. Have a look!

vstinner · 2017-06-29T13:43:41Z

Doc/library/datetime.rst

-| ``repr(t)``                    | Returns a string in the form                  |
-|                                | ``datetime.timedelta(D[, S[, U]])``, where D  |
-|                                | is negative for negative ``t``. (5)           |
-+--------------------------------+-----------------------------------------------+


Prefer don't remove documentation, but update it. If you don't want to describe the format, just say something like "Returns the representation of the time delta as a string."

Sounds reasonable. Done.

vstinner · 2017-06-29T13:45:37Z

Lib/test/datetimetester.py

+        self.assertEqual(repr(self.theclass(days=1, microseconds=100)),
+                         "%s(days=1, microseconds=100)" % name)
+        self.assertEqual(repr(self.theclass(seconds=1, microseconds=100)),
+                         "%s(seconds=1, microseconds=100)" % name)


I don't think that it's worth it to add so many test cases. Just keep 3 examples, 1 negative value, the timedelta(0), and you're done no?

Some of the tests were added to differentiate the current implementation from the old one: i.e. no leading days=0 or seconds=0 and to highlight the current handling of negative values (i.e. the normalization of the attributes, as described in the docs). I'll remove the ones which don't test anything unique not covered elsewhere.

vstinner · 2017-06-29T13:45:57Z

Lib/test/test_datetime.py

@@ -20,7 +20,7 @@
 # XXX(gb) First run all the _Pure tests, then all the _Fast tests.  You might
 # not believe this, but in spite of all the sys.modules trickery running a _Pure
 # test last will leave a mix of pure and native datetime stuff lying around.
-test_classes = []
+all_test_classes = []


Please fix the loop rather than renaming this variable.

vstinner · 2017-06-29T13:46:46Z

Modules/_datetimemodule.c

+        if (microseconds == NULL || PyList_Append(args, microseconds) < 0) {
+            Py_DECREF(args);
+            Py_XDECREF(microseconds);
+            return NULL;


Please move "Py_DECREF(args); return NULL;" to a new "error:" label at the end, and use "goto error;" here, to factorize the code (duplicated 3 times).

vstinner · 2017-06-29T14:29:58Z

Lib/datetime.py

+        if self._microseconds:
+            args.append("microseconds=%d" % self._microseconds)
+
+        if len(args) == 0:


Replace this with:

if not args:
args.append('0')

to prevent code duplication.

"if not args:" is better

vstinner · 2017-06-29T14:30:26Z

Modules/_datetimemodule.c

+        Py_DECREF(microseconds);
+    }
+
+    if (PyList_Size(args) == 0) {


Please replace PyList_Size() with PyList_GET_SIZE().

PyList_Size() can fail, PyList_GET_SIZE() cannot.

vstinner · 2017-06-29T14:31:05Z

Modules/_datetimemodule.c

+    }
+
+    PyObject *repr = PyUnicode_FromFormat("%s(%S)",
+                                          Py_TYPE(self)->tp_name,


I don't recall if you reply to my question: does this include the module name? Does it use the qualified name?

Yes, it uses the fully qualified name. Otherwise, the tests would have caught it. I did reply to it, but Github probably collapsed the comment.

Ok, cool :-)

vstinner · 2017-06-29T14:40:02Z

Doc/library/datetime.rst

-| ``str(t)``                     | Returns a string in the form                  |
-|                                | ``[D day[s], ][H]H:MM:SS[.UUUUUU]``, where D  |
-|                                | is negative for negative ``t``. (5)           |
+| ``str(t)``                     | Returns a string in the form                  | |                                | ``[D day[s], ][H]H:MM:SS[.UUUUUU]``, where D  | |                                | is negative for negative ``t``. (5)           |


This line seems weird. Did you try to compile the doc? make -C Doc html

Why did you touch this doc?

Ah, crap. This was an illusion created by Vim's soft-wrapping. git add -p too broke the lines weirdly so I wasn't able to catch it. Am running make html now to verify the output. Sorry about missing this.

vstinner · 2017-06-29T14:40:36Z

Lib/datetime.py

+        if self._microseconds:
+            args.append("microseconds=%d" % self._microseconds)
+
+        if len(args) == 0:


"if not args:" is better

vstinner · 2017-06-29T14:41:08Z

Lib/datetime.py

-                                          self._seconds,
-                                          self._microseconds)
+        args = []
+


IMHO the empty lines doesn't help readability, you can remove them in all the code filling args.

vstinner · 2017-06-29T14:41:23Z

Lib/test/test_datetime.py

@@ -20,7 +20,7 @@
 # XXX(gb) First run all the _Pure tests, then all the _Fast tests.  You might
 # not believe this, but in spite of all the sys.modules trickery running a _Pure
 # test last will leave a mix of pure and native datetime stuff lying around.
-test_classes = []
+all_test_classes = []


vstinner · 2017-06-29T14:42:14Z

Modules/_datetimemodule.c

+        Py_DECREF(microseconds);
+    }
+
+    if (PyList_Size(args) == 0) {


Please replace PyList_Size() with PyList_GET_SIZE().

PyList_Size() can fail, PyList_GET_SIZE() cannot.

vstinner · 2017-06-29T14:42:29Z

Modules/_datetimemodule.c

+
+    if (PyList_Size(args) == 0) {
+        PyObject *zero = PyUnicode_FromString("0");
+


remove this useless empty line

vstinner · 2017-06-29T14:42:47Z

Modules/_datetimemodule.c

+    }
+
+    PyObject *repr = PyUnicode_FromFormat("%s(%S)",
+                                          Py_TYPE(self)->tp_name,


Ok, cool :-)

vstinner · 2017-06-29T14:43:19Z

Modules/_datetimemodule.c

+    PyObject *args_string = PyUnicode_Join(sep, args);
+    Py_DECREF(sep);
+    Py_DECREF(args);
+


You might remove this empty line for readability, collapse the logical block.

vstinner

LGTM, but I would like to see a review of @serhiy-storchaka and/or @vadmium since we had some disagreement on the repr() output.

serhiy-storchaka

The C code looks correct to me, but a little cumbersome. See my comment on the tracker.

Please add your name in Misc/ACKS if it still is not added and an entry in Misc/NEWS.d/.

serhiy-storchaka · 2017-06-30T18:01:40Z

Lib/test/test_datetime.py

@@ -20,7 +20,7 @@
 # XXX(gb) First run all the _Pure tests, then all the _Fast tests.  You might
 # not believe this, but in spite of all the sys.modules trickery running a _Pure
 # test last will leave a mix of pure and native datetime stuff lying around.
-test_classes = []
+all_test_classes = []


Because the tests were only running for the C implementation, not for the Python
implementation.

This looks like a bug. Please open a new issue for it. This should be fixed in all versions.

serhiy-storchaka · 2017-06-30T18:16:16Z

Doc/library/datetime.rst

-| ``repr(t)``                    | Returns a string in the form                  |
-|                                | ``datetime.timedelta(D[, S[, U]])``, where D  |
-|                                | is negative for negative ``t``. (5)           |
+| ``repr(t)``                    | Returns the representation of the             |


It is not clear how this representation differs from the above one. May be add "in the calling form" or something like?

Returns the representation of the :class:timedelta object such that t == eval(repr(t)).

?

vstinner

Still LGTM with the new C code ;-)

serhiy-storchaka

Yet few nitpicks.

serhiy-storchaka · 2017-07-01T04:36:54Z

Lib/test/test_datetime.py

@@ -20,7 +20,7 @@
 # XXX(gb) First run all the _Pure tests, then all the _Fast tests.  You might
 # not believe this, but in spite of all the sys.modules trickery running a _Pure
 # test last will leave a mix of pure and native datetime stuff lying around.
-test_classes = []
+all_test_classes = []


After merging the PR that fixes the tests this PR should be merged with master. Merge conflicts that are not resolved automatically should be resolved manually.

serhiy-storchaka · 2017-07-01T04:37:30Z

Modules/_datetimemodule.c

+        return NULL;
+    }
+
+    char *sep = "";


const char*

serhiy-storchaka · 2017-07-01T04:38:58Z

Modules/_datetimemodule.c

+
+    if (GET_TD_SECONDS(self) != 0) {
+        Py_SETREF(args, PyUnicode_FromFormat("%U%sseconds=%d",
+                                             args,


I think all formatted arguments may be written in one line. This is more compact.

serhiy-storchaka · 2017-07-01T04:42:13Z

Misc/ACKS

@@ -1762,3 +1762,4 @@ Jelle Zijlstra
 Gennadiy Zlobin
 Doug Zongker
 Peter Åstrand
+Utkarsh Upadhyay


Names are ordered in alphabetical order. Your name should be under the U letter.

musically-ut · 2017-07-03T08:45:02Z

Doc/library/datetime.rst

-|                                | is negative for negative ``t``. (5)           |
+| ``repr(t)``                    | Returns a string representation of the        |
+|                                | :class:`timedelta` object such that           |
+|                                | ``t == eval(repr(t))``.                       |
 +--------------------------------+-----------------------------------------------+


t == eval(repr(t)) isn't necessarily true (e.g. when datetime is aliased during import). So this part of the documentation would need to change again.

And remove a duplicate test-case.

Also, DRY the code and remove unnecessary test cases.

len(args) == 0 -> not args PyList_Size -> PyList_GET_SIZE

Also, fix some whitespace.

musically-ut · 2017-07-25T18:01:49Z

I've rebased the PR on the current master.

This is a fix to the documentation to reflect the changes merged in as part of python#1493 (bpo-30302).

the-knights-who-say-ni added the CLA not signed label May 7, 2017

Mariatta removed the CLA not signed label May 8, 2017

the-knights-who-say-ni added the CLA signed label May 8, 2017

musically-ut force-pushed the feat-timedelta-repr branch from fbf6352 to 39d2352 Compare June 27, 2017 12:09

vstinner requested changes Jun 27, 2017

View reviewed changes

serhiy-storchaka requested changes Jun 27, 2017

View reviewed changes

serhiy-storchaka reviewed Jun 27, 2017

View reviewed changes

vstinner requested changes Jun 29, 2017

View reviewed changes

vstinner reviewed Jun 29, 2017

View reviewed changes

vstinner approved these changes Jun 29, 2017

View reviewed changes

serhiy-storchaka reviewed Jun 30, 2017

View reviewed changes

vstinner approved these changes Jun 30, 2017

View reviewed changes

serhiy-storchaka reviewed Jul 1, 2017

View reviewed changes

musically-ut mentioned this pull request Jul 1, 2017

bpo-30822: Fix testing of datetime module. #2530

Merged

2 tasks

musically-ut commented Jul 3, 2017

View reviewed changes

musically-ut added 8 commits July 25, 2017 19:51

Make timedelta.__repr__ more informative.

1cb7b91

Make C impl of timedelta.__repr__ more informative

84241de

Fix tests.

2e61096

Add more tests.

e6970c8

Do not show args which are 0.

f5dd914

Fix bug to run tests on both C and Python code.

e1c33ab

Use append/join instead of concatenating strings.

345557b

Remove unsed tests.

e79d223

musically-ut added 18 commits July 25, 2017 19:51

Use append/join in C implementation.

312b81b

Remove documentation of repr.

1ca30ee

Do stricter reference management, remove refleaks.

f643b40

Inline simple expression.

194bfde

Add checks for C-API failures.

a40e0c1

Make DECREFs more concise.

69213f7

Add more tests.

c939f81

And remove a duplicate test-case.

Update documentation for repr.

9e7bd21

timedelta(seconds=0) -> timedelta(0)

863485c

Also, DRY the code and remove unnecessary test cases.

Make the code more DRY.

5d5a92e

Remove empty lines and use idiomatic methods.

16516a4

len(args) == 0 -> not args PyList_Size -> PyList_GET_SIZE

Revert white-space regression in documentation.

2fa3e20

Update ACKS and NEWS.d.

10817a8

Use PyUnicode_* instead of PyList_*.

e26ab32

Make the invariant in case of repr explicit.

83ea587

Place name at the correct alphabetical position.

23eb9ec

Update documentation.

ae93f8e

char * -> const char*

3d327f4

Also, fix some whitespace.

musically-ut force-pushed the feat-timedelta-repr branch from 46f0d0c to 3d327f4 Compare July 25, 2017 18:00

vstinner merged commit cc5a65c into python:master Jul 25, 2017

a12233 mentioned this pull request Jul 29, 2017

Merge pull request #1 from python/master #2943

Closed

musically-ut added a commit to musically-ut/cpython that referenced this pull request Sep 21, 2017

Update documentation containing timedeltas repr.

79446b5

This is a fix to the documentation to reflect the changes merged in as part of python#1493 (bpo-30302).

musically-ut mentioned this pull request Sep 21, 2017

bpo-31545 Update documentation containing timedeltas repr. #3687

Merged


		if (PyList_Size(args) == 0) {
		PyObject *zero = PyUnicode_FromString("0");

Uh oh!

bpo-30302 Make timedelta.__repr__ more informative. #1493

bpo-30302 Make timedelta.__repr__ more informative. #1493

Uh oh!

Conversation

musically-ut commented May 7, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented May 7, 2017

Uh oh!

mention-bot commented May 7, 2017

Uh oh!

vadmium commented May 7, 2017

Uh oh!

musically-ut commented May 7, 2017

Uh oh!

musically-ut commented May 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

musically-ut Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpo-30302 Make timedelta.repr more informative. #1493

bpo-30302 Make timedelta.repr more informative. #1493

musically-ut commented May 7, 2017 •

edited by bedevere-bot

Loading

musically-ut commented May 8, 2017 •

edited

Loading

musically-ut Jun 27, 2017 •

edited

Loading