Improve .__repr__ implementation for datetime.timedelta

[musically-ut]

BPO	30302
Nosy	@tim-one, @abalkin, @vstinner, @bitdancer, @berkerpeksag, @vadmium, @serhiy-storchaka, @musically-ut
PRs	#1493 #2929 #2943 #3687

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-10-27.11:28:20.761>
created_at = <Date 2017-05-07.19:57:26.207>
labels = ['3.7', 'type-feature', 'library']
title = 'Improve .__repr__ implementation for datetime.timedelta'
updated_at = <Date 2017-10-27.11:28:20.759>
user = 'https://github.com/musically-ut'

bugs.python.org fields:

activity = <Date 2017-10-27.11:28:20.759>
actor = 'berker.peksag'
assignee = 'none'
closed = True
closed_date = <Date 2017-10-27.11:28:20.761>
closer = 'berker.peksag'
components = ['Library (Lib)']
creation = <Date 2017-05-07.19:57:26.207>
creator = 'musically_ut'
dependencies = []
files = []
hgrepos = []
issue_num = 30302
keywords = ['patch']
message_count = 35.0
messages = ['293212', '293213', '293215', '293216', '293219', '294062', '294106', '297029', '297048', '297049', '297050', '297269', '297293', '297424', '297429', '297438', '297440', '297441', '297444', '297447', '297457', '297471', '297521', '297531', '297560', '297561', '297570', '297865', '299148', '299149', '299152', '299381', '299384', '305111', '305113']
nosy_count = 8.0
nosy_names = ['tim.peters', 'belopolsky', 'vstinner', 'r.david.murray', 'berker.peksag', 'martin.panter', 'serhiy.storchaka', 'musically_ut']
pr_nums = ['1493', '2929', '2943', '3687']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue30302'
versions = ['Python 3.7']

[musically-ut]

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)

?

[serhiy-storchaka]

The drawback is that this change increases the length of the repr. If you output few values in a row (for example output the repr of a list of timedeltas), this makes the output less readable.

Users of datetime.timedelta know what arguments mean. If they don't know they always can look in the documentation or builtin help.

datetime.datetime has more arguments, and its repr doesn't use keywords.

[musically-ut]

The drawback is that this change increases the length of the repr.

I would argue that it is a reasonable trade-off given the increase in ease of understanding.

I know that this is a weak argument, but, keywords are not without precedent. Consider the comically more verbose example:

import time
time.gmtime(1121871596)
# time.struct_time(tm_year=2005, tm_mon=7, tm_mday=20, tm_hour=14, tm_min=59, tm_sec=56, tm_wday=2, tm_yday=201, tm_isdst=0)

datetime.datetime has more arguments, and its repr doesn't use keywords.

I think that guessing the meaning of values is much harder when it comes to timedelta.

Users of datetime.timedelta know what arguments mean. If they don't know they always can look in the documentation or builtin help.

I created the issue after ... a friend ... spent an embarrassing amount of time debugging because he thought that the third argument represented milliseconds and not microseconds. <_<

I could, of course, tell him:

In the face of ambiguity, resist the temptation to guess.

But he could retort:

Explicit is better than implicit.

and

Readability counts.

I think he has a point.

[vadmium]

This was discussed fairly recently: <https://marc.info/?i=CAPTjJmrBxpvYQUYXshBC1J13M_h5or67CNBKrkySw4ef6RqDoQ@mail.gmail.com\>. There seems to be a bit of support for changing this. It is not a bug fix, so has to go into the next release, now 3.7.

I disagree that the positional timedelta parameters are well-known.

The built-in help was also discussed in that thread. I don’t think it got fixed yet, did it?

The datetime class represents absolute dates. It is nonsense to specify a date without a year. Timedelta is different, because time is measured with different units. The first parameter (days) is surprising considering most other Python APIs (sleep etc) deal with seconds.

The size of the repr could be reduced a bit by dropping the module name: datetime.timedelta vs just timedelta. Although that would be inconsistent with the other classes; I’m not sure about this.

Other improvements that I would like to see:

1. Drop leading zeros: timedelta(seconds=60) rather than timedelta(days=0, seconds=60). This would also help reduce the size.

2. Factor out the negative sign: -timedelta(seconds=60) rather than timedelta(days=-1, seconds=86340).

[musically-ut]

This was discussed fairly recently: <https://marc.info/?i=CAPTjJmrBxpvYQUYXshBC1J13M_h5or67CNBKrkySw4ef6RqDoQ@mail.gmail.com\>

That thread went deep and culminated here, as far as I can tell: https://marc.info/?l=python-dev&m=145077422417470&w=2 (I may not have explored all the branches, though.)

So there indeed seems to be general agreement about changing this. It was heartening to know that I wasn't the only one to stumble. \o/

The built-in help was also discussed in that thread. I don’t think it got fixed yet, did it?

No, the doc-string is as uninformative as then, as far as I can tell:

In [178]: datetime.timedelta?
Init signature: datetime.timedelta(self, /, *args, **kwargs)
Docstring: Difference between two datetime values.
File: ~/miniconda3/lib/python3.5/datetime.py
Type: type

I'll investigate what documentation for other functions looks like and see if I can come up with something better. The exact documentation would be best discussed over diffs on Github.

Then there is the issue of repr being explicitly documented, as you had pointed out on the Github issue. Guido thinks that any breakage is _unlikely_ but "asked around" here: https://marc.info/?l=python-dev&m=145065347022557&w=2

As far as I can tell, he didn't see an explicit response.

The size of the repr could be reduced a bit by dropping the module name: datetime.timedelta vs just timedelta. Although that would be inconsistent with the other classes; I’m not sure about this.

Personally, I don't see a big problem either way but having datetime.timedelta in the repr feels reassuring to me that I have the 'right' type instead of some other 'timedelta' from a non-stdlib module (e.g. moments or pandas).

1. Drop leading zeros: timedelta(seconds=60) rather than timedelta(days=0, seconds=60). This would also help reduce the size.

This sounds reasonable. I'll make this change and add corresponding tests.

2. Factor out the negative sign: -timedelta(seconds=60) rather than timedelta(days=-1, seconds=86340).

I'm not sure there was consensus about this; if I understand correctly, Guido thinks that it is important that the repr return what the attributes hold: https://marc.info/?l=python-dev&m=145066934224955&w=2

[vadmium]

I think the benefit of the repr being easier to understand outweighs the pain of breaking the old format. If the change is a problem, that might be mitigated by adding an entry to the “Porting to Python 3.7” documentation.

I don’t think my option of factoring the minus sign out to the front of the timedelta constructor was mentioned on Python-dev. The advantage is that it doesn’t mention problematic negative attribute values, and if you negate a timedelta, you still get the right attribute values:

>>> d
-datetime.timedelta(seconds=60)
>>> -d
datetime.timedelta(seconds=60)
>>> (-d).seconds
60

[musically-ut]

So we are okay with

datetime.timedelta(minutes=-1)
# -datetime.timedelta(seconds=60)

and

datetime.timedelta(minutes=-1).seconds
\bpo-86340

datetime.timedelta(minutes=-1).days
# -1

?

Perhaps I'm reading this incorrectly:

"If the repr() shows different numbers than the attributes things are worse than now." ~ Guido, https://marc.info/?l=python-dev&m=145066934224955&w=2

Maybe he was only talking about:

datetime.timedelta(minutes=-1)
# datetime.timedelta(minutes=-1)

but I would like a second opinion on it. :)

Also, I'll just drop the description of .repr from the .RST documentation because it is not to be relied on anyway. Any objections to that?

~
ut

[musically-ut]

I've added the following tests to remove the 0 attributes from the repr:

    self.assertEqual(repr(self.theclass(days=1, seconds=0)),
                     "%s(days=1)" % name)
    self.assertEqual(repr(self.theclass(seconds=60)),
                     "%s(seconds=60)" % name)
    self.assertEqual(repr(self.theclass()),
                     "%s(seconds=0)" % name)
    self.assertEqual(repr(self.theclass(microseconds=100)),
                     "%s(microseconds=100)" % name)
    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 am still ambivalent about factoring the minus sign outside, though.

I've also fixed a bug in test_datetime.py which prevented execution of the Python implementation in Lib/datetime.py.

TODO:

• Decide about factoring the minus sign.
• Drop description of timedelta.__repr__ from the documentation.

~
ut

[vstinner]

2. Factor out the negative sign: -timedelta(seconds=60) rather than timedelta(days=-1, seconds=86340).

Please don't do that. repr() is designed for developers, not end users. Don't make repr() implementation overcomplicated. Just dump raw data, tha's all.

If you want something more smart, use str() or even write a *new* method?

[vstinner]

What is your favorite repr() for datetime.timedelta(seconds=0)?

Since it's hard to decide which unit is the best, I suggest to not write any unit: either "datetime.timedelta(0)" or "datetime.timedelta()" is fine with me.

In fact, "datetime.timedelta(seconds=0)" is also ok, since seconds is the most common unit, no?

(Ok, to be honest, I don't really care. But I wanted to have a discussion on this issue ;-))

[musically-ut]

@Haypo: thanks for the input!

I will find datetime.timedelta() to be rather confusing because it does not explicitly tell me that the interval indeed is 0.

Between datetime.timedelta(0) and datetime.timedelta(seconds=0), I am ambivalent but I prefer the latter for consistency with the other repr.

[vstinner]

Between datetime.timedelta(0) and datetime.timedelta(seconds=0), I am ambivalent but I prefer the latter for consistency with the other repr.

I read your latest PR. Now I don't like seconds=0 anymore. I would prefer a datetime.timedelta(0) special case.

In the Python implementation, add:

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

and remove the "not days and not microseconds" special case.

datetime.timedelta(0) was always valid, and IMHO it's explicitly enough.

I'm also ok with "datetime.timedelta()" alone, since it was also always accepted and is also well defined.

[musically-ut]

I went through the e-mail chain discussing the changes in repr again and I have come around to seeing that though Guido was somewhat opposed to the idea of factoring out the minus sign because it would have meant changing the attributes 1, what he really didn't want was perhaps adding attributes in repr which don't exist on the object 2. Changing to keyword arguments was a 'go', though 3.

Esp. after it has been pointed out how easy it would be do implement, my opposition to the idea of factoring out the negative sign has somewhat come down. Personally, I prefer the current version.

Further, this PR has also come to fix a couple of issues in the testing framework and it is becoming rather unwieldy to throw in doc-string changes into it as well. Hence, after some discussion with @Haypo, I think I'll make a separate PR for fixing the docstrings after this PR is merged; there was general consensus that the docstrings should be fixed [4,5]. @Haypo has, thankfully, showed me how and where to do that using Argument Clinic, which was the show-stopper the last time 6.

Thoughts and opinions?

~
ut

[bitdancer]

I'm not entirely sure what you are asking for opinions on, but for what it is worth I'm with Haypo: the repr should show the *actual* value of the attributes.

[musically-ut]

@r.david.murray: I'm primarily waiting for Serhiy (and, optionally Martin) to "Okay" the pull request.

The code seems fine (@Haypo has had a through look at it), but we still were mildly divided over whether we want to factor out the negative sign or not.

re: thoughts and opinions; I wanted to have them on:

• Whether to factor out the negative sign.
• Whether to throw in the doc-string/documentation changes into this PR or in a new one.

Current position on these questions:

• In favor of factoring out the -ve sign: Martin
  Not in favor of factoring out the -ve sign: Victor, R. David

• DocString changes: (@Haypo's opinion) in a separate PR.

~
ut

[bitdancer]

If the docstring changes incorporate changes from this PR, I'd keep them in a single PR myself. If not, two PRs would make review easier. On the other hand, if haypo is volunteering to do the review, do whatever he wants :)

[musically-ut]

The docstring changes ought to be easier to review since I'll only be making the Python and C docstrings mostly identical (with some minor changes here and there). I'll also (hopefully) need fewer pointers now that I've been through one review process (thanks @Haypo!).

To answer your question, the changes in this PR are completely orthogonal to the changes in the docstring needed.

Hence, I'm counting one more vote towards making the docstring changes in a separate PR.

Thanks! :-)

~
ut

[serhiy-storchaka]

Don't wait me. I have no preferences and just remind about the Martin's suggestion.

The C code looks cumbersome. It could be made a little simpler if accumulate arguments into a string rather than a list.

    if (GET_TD_SECONDS(self) != 0) {
        Py_SETREF(args, PyUnicode_FromFormat("%U%sseconds=%d",
                                             args, sep, GET_TD_SECONDS(self)));
        if (args == NULL)
            return NULL;
        sep = ", ";
    }

args is initialized by an empty Python string, sep is initialized by "". PyUnicode_Join() at the end is not needed. This would save more than 20 lines.

The code could be simplified even more if use the char buffer(s) and PyOS_snprintf instead of PyUnicode_FromFormat().

[vstinner]

Serhiy: "The C code looks cumbersome. It could be made a little simpler if accumulate arguments into a string rather than a list."

Utkarsh's first version used C code. I proposed to use a PyList and PyUnicodeObject strings to not have to compute the size of the char[] buffers.

The annoying part is to handle the ", " separator. I also had bad experiences with handling char* strings. It's so easy to make mistakes :-(

IMHO the current C code using Python objects is not that complex, but I don't really care of the implementation as soon as it works :-)

[musically-ut]

So, as a compromise, I'll stick to PyObjects (avoiding char* and the buffer size calculations) but will use PyUnicode all the way instead using PyList_* functions (thus reducing the size of the code by ~20 lines).

I've pushed this change, ready for review! :)

(haypo has already approved the change while I am still writing this.)

~
ut

[vadmium]

Don’t let my minus sign suggestion stop this, especially since a couple other people said they don’t like it. The current pull request proposal is beneficial without it.

Isn’t there a “Unicode writer” API that could be used? Maybe that’s another alternative to the string accumulation or list building, but I never used it so it may not be appropriate.

[serhiy-storchaka]

Isn’t there a “Unicode writer” API that could be used?

It could be used if accumulate only Python strings, C strings and separate characters. But in any case we need to convert C integers to strings, and add up to 3 items per every attribute (separator, name, value), the result of every addition should be checked. PyUnicode_FromFormat() is an easy way to concatenate several items. It needs only one check.

The annoying part is to handle the ", " separator. I also had bad experiences with handling char* strings. It's so easy to make mistakes :-(

It could be simpler if use the trick with the sep variable. As for mistakes, actually that version of Utkarsh's patch was less buggy than the few following versions using the Python C API. But the final C code is correct and LGTM.

[abalkin]

Sorry for being late to the discussion, but please allow me to add a -1 vote. The time.struct_time precedent is indeed comically verbose. Whenever I need to inspect a struct_time, I cast it to a plain tuple

Compare

>>> time.gmtime(1121871596)
time.struct_time(tm_year=2005, tm_mon=7, tm_mday=20, tm_hour=14, tm_min=59, tm_sec=56, tm_wday=2, tm_yday=201, tm_isdst=0)

and

>>> time.gmtime(1121871596)[:]
(2005, 7, 20, 14, 59, 56, 2, 201, 0)

Unless you need to know what the last three fields are, the long form gives no advantage.

datetime.timedelta(days=3114, seconds=28747, microseconds=100000) is not as verbose and the extra information may be helpful the first time you see it, but if you deal with timedeltas a lot, it would quickly become annoying. Moreover, unlike in the struct_time case, there will be no easy way to suppress metadata.

Furthermore, "seconds=28747" is not that user-friendly. A friendlier representation would be "hours=7, minutes=59, seconds=7" and similar information is displayed when you print a timedelta:

>>> datetime.timedelta(days=3114, seconds=28747, microseconds=100000)
datetime.timedelta(3114, 28747, 100000)
>>> print(_)
3114 days, 7:59:07.100000

[serhiy-storchaka]

I don't have very strong preferences, but I concur with Alexander. His arguments match my initial opinion about this issue (msg293213).

[vstinner]

Furthermore, "seconds=28747" is not that user-friendly. A friendlier representation would be "hours=7, minutes=59, seconds=7" and similar information is displayed when you print a timedelta: (...)

I agree that seconds=28747 is not that user-friendly, *but* maybe it shows a flaw in timedelta design?

Maybe timedelta should only expose properties which would *compute* hours, minutes, etc. from an internal storage? But if we change timedelta.seconds value, it is likely to break the backward compatibility. I wrote my own total_seconds() function which uses days, seconds and microseconds fields.

Or maybe we need a new method to convert a timedelta into a more human friendly (named)tuple?

[vstinner]

Alexander: "Sorry for being late to the discussion, but please allow me to add a -1 vote."

While I agree that seconds=28747 is not perfect, IMHO "datetime.timedelta(seconds=28747)" is an enhancement over "datetime.timedelta(0, 28747)".

Can't we make small enhanements step by step, rather than each time try to redesign datetime? ;-)

(Yeah, datetime is now perfect, but well, it's now widely used and part of the stdlib!)

[musically-ut]

[...] the extra information may be helpful the first time you see it, but if you deal with timedeltas a lot, it would quickly become annoying.

It seems from the discussion on the issue that there are two kinds of developers:

• One group which uses timedeltas so often that they will find it annoying if the repr was longer, which takes up more screen space.
• The other group which occasionally uses timedelta but may forget what the arguments stood for.

I suspect that the distribution of programmers who use timedeltas binned by their frequency of usage will follow the Pareto principle: only 20% of the developers will account for 80% of uses of timedelta, and the remaining 80% of the developers will use it the remaining 20% of the time. Out of those 80% developers who use it sparingly, some fraction will find the repr with the keywords more informative than the current version and be a tiny bit happier.

I personally belong to the second group of developers. IMHO, Guido too may belong the second group https://marc.info/?l=python-dev&m=145066335824146&w=2 :

Well it would have saved me an embarrassing moment -- I typed datetime.timedelta(seconds=1e6) at the command prompt and when the response came as datetime.timedelta(11, 49600) I mistook that as 11 years (I was in a hurry and trying hard not to have to think :-).

@Haypo: in the same mail, Guido also talks (jokes?) about improving timedeltas from grounds up:

I might still go for it [i.e. changing the attributes], if it wasn't too late by over a decade (as Tim says).

:)

Though I think the discussion there stopped before anyone could raise the objection about verbosity and clarity, Guido, IMO, was optimistic about this change:

I still think the repr change to use keywords has a good chance for 3.6.

https://marc.info/?l=python-dev&m=145073617109654&w=2

~
ut

[musically-ut]

Bump!

----

>>> time.gmtime(1121871596)[:]
(2005, 7, 20, 14, 59, 56, 2, 201, 0)

I wouldn't mind implementing __getitem__ for timedeltas, such that this becomes possible:

> (D.fromtimestamp(1390953543.1) - D.fromtimestamp(1121871596))[:]
(3114, 28747, 100000)

(Though, TBH, it does feel a little weird that time.struct_time allows indexing).

~
ut

[vstinner]

This issue has a long history starting in 2015 with a thread on python-dev. Many messages were written on python-dev and on this issue. I will try to summarize. (I hate doing that, summarizing is a risk of missing important information or misunderstand someone.)

The issue opens multiple questions:

• Should we change repr?
  ** YES, Guido: "I still think the repr change to use keywords has a good chance for 3.6." and "Still, I can't be the only one to ever have been fooled by this, and it is definitely pretty arcane knowledge what the positional arguments to timedelta()."
  ** YES, Martin Panter: "I disagree that the positional timedelta parameters are well-known."
  ** YES, Victor Stinner
  ** NO, Tim Peter: "But I wouldn't change repr() - the internal representation is fully documented, and it's appropriate for repr() to reflect documented internals as directly as possible."
  ** NO, Serhiy: "The drawback is that this change increases the length of the repr. Users of datetime.timedelta know what arguments mean. If they don't know they always can look in the documentation or builtin help."
  ** NO, Alexander Belopolsky: msg297521

• Can changing repr break code?
  ** Guido: when we randomized hash, a lot of tests failed

• Add a "-" prefix for negative result: -timedelta(seconds=1)?

• (Related) Should repr(timedelta(seconds=60)) returns 'timedelta(minutes=1)'?
  ** NO, Guido: "I'm sure that one often catches people by surprise. However, I don't think we can fix that one without also fixing the values of the attributes -- in that example days is -1 and seconds is 86340 (which will *also* catch people by surprise). And changing that would be much, much harder for backwards compatibility reasons-- we'd have to set days to 0 and seconds to -60, and suddenly we have a much murkier invariant, instead of the crisp"
  ** NO, Guido: "Then please just trust me. If the repr() shows different numbers than the attributes things are worse than now. People will casually look at the repr() and assume they've seen what the attributes will return, and spend hours debugging code that relies on that incorrect assumption."
  ** NO, R. David Murray : "I'm with Haypo: the repr should show the *actual* value of the attributes."
  ** NO, Victor Stinner: "Don't make repr() implementation overcomplicated (...) use str()"

Another remark of Alexander Belopolsky:

• "Furthermore, "seconds=28747" is not that user-friendly. A friendlier representation would be "hours=7, minutes=59, seconds=7" and similar information is displayed when you print a timedelta: (...)"
  ** IHMO you should use str() and not repr() to format a value for humans. repr() is more designed for developers, to debug.

Another interesting remark: the documentation of the constructor is incomplete and should be document parameters.

[vstinner]

New changeset cc5a65c by Victor Stinner (Utkarsh Upadhyay) in branch 'master':
bpo-30302 Make timedelta.__repr__ more informative. (bpo-1493)
cc5a65c

[vstinner]

Do you think that repr(datetime.timedelta) should be documented in the Porting section of What's New in Python 3.7?

https://docs.python.org/dev/whatsnew/3.7.html#porting-to-python-3-7

It was proposed in the middle of the python-dev thread. I have no opinion on that... or just a little -0 vote, if someone is impacted, it should be easy to notice it anymore, no?

--

While we don't have an obvious consensus, I took the responsability of merging Utkarsh Upadhyay's contribution. He wrote that he is interested to pursue the effort by also enhancing the docstring: please, go ahead, write a new PR, I will help you!

It was discussed to rewrite datetime.timedelta: only accept keywords in its constructor, store seconds=60 as minutes=1, etc. The problem is that it's too late to change the design. So I suggest to not waste time on discussing that, but focus the energy on making tiny changes to enhance the implemenatation without risking of breaking the world.

While developers using regulary timedelta knows what means the 3 numbers, it seems clear that the other developers have to check for the documentation. IMHO having to check the documentation is an issue: adding keywords to repr() reduces the need of documentation, and so makes Python more usable.

Sorry about the longer repr(): use str() or write your own formater?

By the way, if you need a formatter working on Python 2.7-3.7:

>>> def format_td(td): return 'timedelta(%s, %s, %s)' % (td.days, td.seconds, td.microseconds)
... 
>>> format_td(datetime.timedelta(seconds=1))
'timedelta(0, 1, 0)'

If someone disagree with my merge, I suggest to reopen a thread on python-dev.

[musically-ut]

Thanks for the merge haypo! \o/

I've also created a PR for adding an entry to 'Porting to Python 3.7' in the documentation; I see no harm in including it in the documentation just-in-case.

[vstinner]

New changeset 8e45318 by Victor Stinner (Utkarsh Upadhyay) in branch 'master':
bpo-30302: Update WhatsNew and documentation. (bpo-2929)
8e45318

[berkerpeksag]

New changeset 843ea47 by Berker Peksag (Utkarsh Upadhyay) in branch 'master':
bpo-31545: Update documentation containing timedelta repr. (GH-3687)
843ea47

[berkerpeksag]

All PRs have been merged in both issues (this and bpo-31545) so I guess this issue can be closed now.