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

RFE: change bool(datetime.time(0, 0, 0)) to evaluate as True #58144

Closed
LakinWecker mannequin opened this issue Feb 3, 2012 · 51 comments
Closed

RFE: change bool(datetime.time(0, 0, 0)) to evaluate as True #58144

LakinWecker mannequin opened this issue Feb 3, 2012 · 51 comments
Labels
3.8 stdlib type-feature

Comments

@LakinWecker
Copy link
Mannequin

@LakinWecker LakinWecker mannequin commented Feb 3, 2012

BPO 13936

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 2014-03-20.23:02:51.235>
created_at = <Date 2012-02-03.20:43:16.223>
labels = ['3.8', 'type-feature', 'library']
title = 'RFE: change bool(datetime.time(0, 0, 0)) to evaluate as True'
updated_at = <Date 2021-11-04.14:30:38.369>
user = 'https://bugs.python.org/LakinWecker'

bugs.python.org fields:

activity = <Date 2021-11-04.14:30:38.369>
actor = 'eryksun'
assignee = 'none'
closed = True
closed_date = <Date 2014-03-20.23:02:51.235>
closer = 'python-dev'
components = ['Library (Lib)']
creation = <Date 2012-02-03.20:43:16.223>
creator = 'Lakin.Wecker'
dependencies = []
files = []
hgrepos = []
issue_num = 13936
keywords = []
message_count = 51.0
messages = ['152556', '152557', '152558', '152559', '152560', '152561', '152562', '152563', '152564', '152565', '152567', '176475', '212732', '212738', '212743', '212771', '212777', '212778', '212779', '212782', '212783', '212784', '212785', '212786', '212787', '212788', '212789', '212790', '212791', '212798', '212799', '212801', '212806', '212807', '212871', '212873', '212877', '212879', '212880', '212881', '212883', '212884', '212886', '212892', '212904', '212911', '212915', '212916', '212921', '213434', '214299']
nosy_count = 0.0
nosy_names = []
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue13936'
versions = ['Python 3.8']

@LakinWecker
Copy link
Mannequin Author

@LakinWecker LakinWecker mannequin commented Feb 3, 2012

midnight is represented by datetime.time(0,0,0). However, this time (unlike all other valid times, including datetime.time(0,0,1)) evalutes to false in if conditions:

import datetime
if datetime.time(0,0,0):
    print "datetime.time(0,0,0) is not a bug!"
else:
    print "datetime.time(0,0,0) is a bug!"

if datetime.time(0,0,1):
    print "datetime.time(0,0,1) is not a bug!"
else:
    print "datetime.time(0,0,1) is a bug!"

@LakinWecker
Copy link
Mannequin Author

@LakinWecker LakinWecker mannequin commented Feb 3, 2012

I'm updating the versions to the ones that I've actually tried it on - this is not an exhaustive search at this point.

@LakinWecker LakinWecker mannequin added stdlib type-bug labels Feb 3, 2012
@bitdancer
Copy link
Member

@bitdancer bitdancer commented Feb 3, 2012

I don't think I would have ever thought of testing a datetime for its truth value. But the behavior you observe is consistent with the rest of Python: 0 is false.

I wonder if this is by design or by accident.

@LakinWecker
Copy link
Mannequin Author

@LakinWecker LakinWecker mannequin commented Feb 3, 2012

Right. I've updated my code to be more correct:

instead of:

if not start_time:
    start_time = default_time

it now reads:

if start_time is None:
    start_time = default_time

which operates correctly and works fine for my case, I just thought it was odd that one time out of all of them evaluates to False. I too wonder if it's by design or not.

It's definitely not documented if it is by design.

@birkenfeld
Copy link
Member

@birkenfeld birkenfeld commented Feb 3, 2012

It must be by design -- someone has implemented a __bool__ (formerly __nonzero__) method; otherwise all objects would be true.

@birkenfeld
Copy link
Member

@birkenfeld birkenfeld commented Feb 3, 2012

BTW, "being a valid time" is not a good argument: 0, "" or False are all valid instances of their types.

@abalkin
Copy link
Member

@abalkin abalkin commented Feb 3, 2012

This is by design. I don't have a reference, but I do remember this being discussed. Suggestions for improving the documentation are welcome.

@tim-one
Copy link
Member

@tim-one tim-one commented Feb 3, 2012

From the docs, at:

http://docs.python.org/library/datetime.html#time-objects

"""
in Boolean contexts, a time object is considered to be true if and only if, after converting it to minutes and subtracting utcoffset() (or 0 if that’s None), the result is non-zero.
"""

@LakinWecker
Copy link
Mannequin Author

@LakinWecker LakinWecker mannequin commented Feb 3, 2012

Although I find it odd, you are all correct.

It is documented. (I don't know how I missed that when I read those docs looking for that exact documentation).

It is consistent with the rest of python.

Do I close this? Do you guys? I'm fine with closing it. Thanks and sorry for the noise.

@tim-one
Copy link
Member

@tim-one tim-one commented Feb 3, 2012

It is odd, but really no odder than "zero values" of other types evaluating to false in Boolean contexts ;-) Closing as "invalid".

@tim-one tim-one closed this as completed Feb 3, 2012
@tim-one tim-one added the invalid label Feb 3, 2012
@LakinWecker
Copy link
Mannequin Author

@LakinWecker LakinWecker mannequin commented Feb 3, 2012

Yeah - good point, and I agree.

@gwrtheyrn
Copy link
Mannequin

@gwrtheyrn gwrtheyrn mannequin commented Nov 27, 2012

I disagree, I think this bug should be reopened and fixed.

A use case that I just ran into: I'm using a Django form with time fields that aren't required, but that are only valid in combination (if there's a open time there has to be a close time).

    if not (self.closed or (self.open_time and self.close_time )):                  
        raise ValidationError("Invalid times")

This leads to a Validation Error when using the times 12:00 and 00:00.

Of course, this case is debatable and can be worked around by using self.open is not None and self.close is not None, but there are even more problems when using the times inside the template.

I'm using the following widespread pattern inside my templates:

<p>Close time: {{ close_time|default:"-" }}</p>

The "default" filter used in this case displays the string "-" if the value on the left side of the | symbol evaluates to False. That makes sense in almost all of the cases.

In the case of the datetime.time(0, 0) object, the default value is displayed, even though datetime.time(0, 0).__str__() results in a valid string (in this case '00:00:00').

(By the way, through these experiments I also found a bug in Django's date formatting template function caused by this inconsistency, which I will report separately.)

I agree that casting time objects to a boolean value doesn't make much sense. But especially because of that, inconsistencies in the resulting boolean value should NOT exist. Yet another argument for this is that in many regions midnight isn't considered 00:00, but 24:00, which would obviously not evaluate to False.

Please fix this. Otherwise it will lead to a whole lot of weird bugs in software using the datetime library.

@shai
Copy link
Mannequin

@shai shai mannequin commented Mar 4, 2014

Just got bit by this.

Tim Peters said: """
It is odd, but really no odder than "zero values" of other types evaluating to false in Boolean contexts.
"""

I disagree. Midnight is not a "zero value", it is just a value. It does not have any special qualities analogous to those of 0, "", or the empty set. Time values cannot be added or multiplied. Midnight evaluting to false makes as much sense as date(1,1,1) -- the minimal valid date value -- evaluating to false (and it doesn't).

It makes perfect sense for timedelta(0) to evaluate to false, and it does. time is different.

Also, while I appreciate this will never be fixed for Python2, the same behavior exists in Python3, where there may still be room for improvement.

I second Danilo Bergen's request. Please reopen.

@pelme
Copy link
Mannequin

@pelme pelme mannequin commented Mar 4, 2014

I agree with Danilo and Shai -- this behavior very surprising. I deal with datetimes a lot, and this bug has bitten me a number of times.

I cannot really think of a single case where "if timeobj:" is useful with the current behavior. It results in a check for "is timeobj midnight or false?"

Would that ever be useful in practice? If you are indeed checking for midnight, surely "if timeobj == time(0, 0):" would be the most explicit and obvious way to do it?

@abalkin
Copy link
Member

@abalkin abalkin commented Mar 4, 2014

Please reopen.

Please bring your case to python-ideas. All developers who commented on this issue agree that it is invalid.

@ncoghlan
Copy link
Contributor

@ncoghlan ncoghlan commented Mar 5, 2014

Rewording the issue title and reopening based on the python-ideas thread. The rationale for making this change is that the current behaviour converts a stylistic problem in checking values against a sentinel via "bool(value)" instead of "value is not None" into a subtle data driven behavioural bug that only occurs exactly at midnight UTC.

If someone wants to write the patch to deprecate this behaviour in Python 3.5 (reporting a deprecation warning whenever midnight is interpreted as False, perhaps suggesting the use of "is" or "is not" instead), and then actually change the behaviour in 3.6, I don't believe we should actively oppose them from doing so.

@ncoghlan ncoghlan changed the title datetime.time(0,0,0) evaluates to False despite being a valid time RFE: change bool(datetime.time(0,0,0)) to evaluate as True Mar 5, 2014
@ncoghlan ncoghlan reopened this Mar 5, 2014
@ncoghlan ncoghlan added type-feature and removed invalid type-bug labels Mar 5, 2014
@ncoghlan
Copy link
Contributor

@ncoghlan ncoghlan commented Mar 5, 2014

Discussion in bpo-20855 made me realise that any deprecation warning should explain how to convert a time object to "seconds since midnight". That model of time of day is the most likely origin of the current behaviour, and explicit conversion to that form would be the easiest way to avoid the deprecation warning in any cases where the current behaviour is actually considered desirable.

@skrah
Copy link
Mannequin

@skrah skrah mannequin commented Mar 5, 2014

I like the current behavior. We have modulo arithmetic here and
bool(96%24) is false, too.

@dstufft
Copy link
Member

@dstufft dstufft commented Mar 5, 2014

It's not modulo arithmetic.

@skrah
Copy link
Mannequin

@skrah skrah mannequin commented Mar 5, 2014

Unix time modulo 86400 gives the number of elapsed seconds in a day
and is zero at midnight. Also, modular arithmetic is colloquially
called "clock arithmetic" for a reason.

@AmberYust
Copy link
Mannequin

@AmberYust AmberYust mannequin commented Mar 6, 2014

Yes, but a datetime.time object is definitely not UNIX time.

@pitrou
Copy link
Member

@pitrou pitrou commented Mar 6, 2014

I agree that having midnight evaluate to false is completely unexpected and unintuitive. I can imagine situations where it bites people in production use in the uncommon case that a time is exactly equal to midnight.

@malemburg
Copy link
Member

@malemburg malemburg commented Mar 6, 2014

On 06.03.2014 10:30, Antoine Pitrou wrote:

Antoine Pitrou added the comment:

I agree that having midnight evaluate to false is completely unexpected and unintuitive. I can imagine situations where it bites people in production use in the uncommon case that a time is exactly equal to midnight.

datetime values with the time set to midnight are *very* common in practice.
You run into them whenever you convert dates into datetime values.

@malemburg malemburg changed the title RFE: change bool(datetime.time(0,0,0)) to evaluate as True RFE: change bool(datetime.time(0, 0, 0)) to evaluate as True Mar 6, 2014
@ncoghlan
Copy link
Contributor

@ncoghlan ncoghlan commented Mar 6, 2014

Donald did some additional testing, and it turns out that it is
specifically midnight *UTC* that is false in boolean context. For a TZ
aware time, the false time depends on the offset (e.g. it would be false at
10 am here in Brisbane).

@dstufft
Copy link
Member

@dstufft dstufft commented Mar 6, 2014

It's actually a bit worse than that Nick. It's midnight UTC, as long as the UTC offset is Positive or Zero. This is because the way the check is implemented is naive.

It's implemented as: Take the time portion sans the tzinfo and convert to minutes, then take the utc offset and convert that to minutes, then subtract the second from the first and if that is zero it is False.

So if you take -5 for instance (my own timezone!) the equation to determine when the "False" time is would look like:

x - (-5 * 60) = 0
x - (-300) = 0
x + 300 = 0
x = -300

So we'd need a time that can be represented as -300 minutes. Since times can not be negative that means for a timezone aware time it is impossible for something with a negative UTC offset to ever be False while for a zero or positive UTC offset it'll be False at UTC midnight.

@ncoghlan
Copy link
Contributor

@ncoghlan ncoghlan commented Mar 6, 2014

I wrote up a longer post on python-ideas regarding the problems that the current behaviour poses when it comes to inferring a correct mental model for datetime.time(): https://mail.python.org/pipermail/python-ideas/2014-March/026647.html

As part of that, it's also worth noting the current behaviour in boolean context is effectively shorthand for:

    import datetime as dt

    naivemidnight = dt.time(0, 0)
    utcmidnight = dt.time(0, 0, tzinfo=dt.timezone.utc
    if x in (naivemidnight, utcmidnight):
        ...

So if the current boolean behaviour is deprecated and removed, it is easily reproduced through equality checks.

It may also make sense to offer an API to easily calculate seconds since midnight, but that would be a separate issue.

@ncoghlan
Copy link
Contributor

@ncoghlan ncoghlan commented Mar 7, 2014

More proposals from the thread (paraphrased):

  • make any aware time() object always True (leave naive midnight as False)
  • make any aware time() object with a non-zero UTC offset always True (leave naive midnight and UTC midnight as False)
  • deprecate aware time() entirely (raises the thorny question of what to return from .time() on an aware datetime() object)
  • add helpers to retrieve naivemidnight and utcmidnight constants, and calculate a localmidnight value (needs to be dynamic in case the local timezone is changed)

Independent observation:

  • if time() objects are supposed to be interpreted as representing a time difference relative to midnight rather than a structured object, why is it so hard to actually convert them to an appropriate time delta? There's no method for it, you can't just subtract midnight, there's no constructor on time delta that accepts a time object, you can't easily attach a date to the time to calculate a time delta.

Use case presented for the current behaviour:

  • a simulation that tracks the time and date of the simulation independently and relies on the implicit bool behaviour of time objects (not stated why this is considered more maintainable than explicit comparisons with appropriate midnight objects)

@ncoghlan
Copy link
Contributor

@ncoghlan ncoghlan commented Mar 7, 2014

Current status of thread discussion (yes, I'm biased, and that shows in the phrasing below):

Arguments raised for status quo:

  • the module is behaving exactly as described in the documentation
  • removing false time values will require affected users to update their code to instead explicitly compare with appropriate midnight values before migrating to Python 3.5 (or, since deprecation warnings are silent by default, except if a test framework enables them, Python 3.6)
  • it wasn't an accident, it was designed so modulo arithmetic could reasonably be implemented for time() objects (which hasn't been demanded or implemented since the datetime module was created)
  • changing behaviour so that a current subtle data driven bug instead becomes a harmless violation of recommended style for comparison against a sentinel value is encouraging bad programming practices

Arguments in favour of changing the behaviour:

  • datetime.time() objects don't behave like a number in any other way (they don't support arithmetic and attempting to convert them with int, float, etc explicitly tells you they're not numbers), and don't even provide an easy way to convert them to a time delta relative to midnight (and hence to "seconds since midnight" via total_seconds), so it's surprising that they behave like a number in boolean context by having a concept of "zero"
  • the current behaviour takes something that would be a harmless style error for most structured data types (including datetime and date objects) and instead makes it a subtle data driven behavioural bug (but only if you're using naive times or a timezone with a non-negative UTC offset)
  • the current behaviour cannot even be accurately summarised as "midnight evaluates as False", because it is actually "naive midnight and UTC midnight with a non-negative UTC offset evaluate as false, while UTC midnight with a negative UTC offset evaluates as true". That's incoherent and really should be changed, and if we're going to change the behaviour anyway, we may as well change it to something less dangerous.
  • any affected code that relies on some variants of midnight being False is already hard to understand (since most readers won't be aware of this subtlety of the behaviour of time objects) and would be made clearer by explicitly comparing against appropriate midnight objects

@smontanaro
Copy link
Contributor

@smontanaro smontanaro commented Mar 7, 2014

the current behaviour takes something that would be a harmless style error for most structured data types ...

I'm not sure what a "structured data type" is, but in my mind the original poster's construct is more than a style error. He was using None as a sentinel, but not explicitly testing for its presence. The same error would be present if he used None as a sentinel value where the range of possible values was the set of all integers.

If there are problems with the definition of "false time" such that there are some combinations of time and timezone where UTC midnight is not zero, I would prefer to correct them.

Further, playing the devil's advocate, if you dispense with any false elements of time objects, why not simply zero out the nb_nonzero slot in time_as_number? Once that's gone, the time_as_number structure is all zeros, so the tp_as_number slot in PyDateTime_TimeType can be cleared.

@ncoghlan
Copy link
Contributor

@ncoghlan ncoghlan commented Mar 7, 2014

Structured data is just a shorthand way of referring to any Python object
which is neither a number or a container and exhibits the default boolean
behaviour where all instances are true.

The problem datetime.time is both that its current behaviour is internally
incoherent (whether or not an aware time is false depends on the current
timezone in unpredictable ways) and *also* inconsistent with its other
behaviours that indicate it should be handled as a non-numeric value. Since
it isn't a container either, standard conventions suggest that it should
always be true. No *compelling* justifications for its atypical behaviour
have been presented, just a case of Tim wanting to leave the door open to
adding modular arithmetic directly on time instances.

I suggest it makes far more sense to instead eliminate the quirky behaviour
entirely and instead provide an easy way to convert a time to a timedelta
relative to midnight.

@bitdancer
Copy link
Member

@bitdancer bitdancer commented Mar 7, 2014

it wasn't an accident, it was designed so modulo arithmetic could reasonably be implemented for time() objects (which hasn't been demanded or implemented since the datetime module was created)

Ah, interesting. I just wrote a program last month where I was baffled that time didn't support arithmetic, and had to dodge painfully through datetime instances to do the arithmetic. I asked about it on IRC and someone said it was because arithmetic on times was ambiguous because of timezones, and I just accepted that rather than wonder why it hadn't been implemented.

Otherwise I'm pretty sympathetic to the RFE, but I'd really like time arithmetic to work, so I guess I'd have to be -1 in that case, wouldn't I?

@pitrou
Copy link
Member

@pitrou pitrou commented Mar 7, 2014

Otherwise I'm pretty sympathetic to the RFE, but I'd really like time
arithmetic to work, so I guess I'd have to be -1 in that case,
wouldn't I?

Adding times of the day sounds as well-defined to me as adding
centigrade temperatures.

@bitdancer
Copy link
Member

@bitdancer bitdancer commented Mar 7, 2014

As does adding dates. I'm talking about timedelta arithmetic, just like for datetimes. I believe that still requires modulo arithmetic :)

@AlexanderBelopolsky
Copy link
Mannequin

@AlexanderBelopolsky AlexanderBelopolsky mannequin commented Mar 7, 2014

On Mar 7, 2014, at 10:12 AM, "R. David Murray" <report@bugs.python.org> wrote:

I asked about it on IRC and someone said it was because arithmetic on times was ambiguous because of timezones, and I just accepted that rather than wonder why it hadn't been implemented.

Otherwise I'm pretty sympathetic to the RFE, but I'd really like time arithmetic to work, so I guess I'd have to be -1 in that case, wouldn't I?

See http://bugs.python.org/issue17267

@AlexanderBelopolsky
Copy link
Mannequin

@AlexanderBelopolsky AlexanderBelopolsky mannequin commented Mar 7, 2014

On Mar 7, 2014, at 10:15 AM, Antoine Pitrou <report@bugs.python.org> wrote:

Adding times of the day sounds as well-defined to me as adding
centigrade temperatures.

What is wrong with adding temperatures? Climate people do it all the time when computing the averages.

@tim-one
Copy link
Member

@tim-one tim-one commented Mar 7, 2014

[Nick]

  • deprecate aware time() entirely (raises the thorny question of what to return from .time() on an aware datetime() object)

aware_datetime_object.time() already returns a naive time object. The thorny question is what .timetz() should return - but if aware time objects _were_ deprecated, .timetz() itself would presumably be deprecated too.

... you can't easily attach a date to the time to calculate a time delta.

The class constructor datetime.combine(date_object, time_object) makes it easy to combine any two date and time objects into a datetime object.

@ethanfurman
Copy link
Member

@ethanfurman ethanfurman commented Mar 7, 2014

If no one else has gotten to this in the next six months or so, I will. :)

@westleymartinez
Copy link
Mannequin

@westleymartinez westleymartinez mannequin commented Mar 7, 2014

So is the plan to deprecate this in 3.5 and remove in 3.6? If so, the question is where should the deprecation be thrown?

@BreamoreBoy
Copy link
Mannequin

@BreamoreBoy BreamoreBoy mannequin commented Mar 8, 2014

There is no plan, other than the BDFL asking for a survey of what is happening with code that relies on this in the real world. FTR I'm completely against this change. I see no reason to change something that's been in use for maybe nine years and does what it's documented to do, based on somebody's expectations, failure to read the documents and buggy code. To me it would be a nail in the coffin of Python's very conservative, and to me extremely proper, view of maintaining backward compatibility.

@dstufft
Copy link
Member

@dstufft dstufft commented Mar 8, 2014

To be specific, Guido said that if this 3.0 or 3.1 he'd be all for changing it, and the only question in his mind is how safe it is change. And that his intuition is that it's a nuisance feature and few people have actually relied on it and that he'd be OK with fixing (and breaking) it in 3.5, perhaps after a thorough search for how often the feature is actually relied on and how legitimate those uses are. (See the full response at https://mail.python.org/pipermail/python-ideas/2014-March/026785.html)

@ncoghlan
Copy link
Contributor

@ncoghlan ncoghlan commented Mar 8, 2014

Mark, we kinda proved we're willing to break backwards compatibility in the name of improving usability when we embarked down the path of creating Python 3 and an associated transition plan from Python 2, rather than just continuing to develop Python 2.

Compared to some of the backwards compatibility breaks within the Python 2 series that were handled using the normal deprecating cycle (removing string exceptions, anyone?), this one would be truly trivial.

@fandingo
Copy link
Mannequin

@fandingo fandingo mannequin commented Mar 13, 2014

This behavior conflicts with the other major classes, datetime.date and datetime.datetime. The ostensible reason for this falsy behavior is that midnight represents a fundamental zero point. We should expect to see similar zero points that evaluate to False for the other two classes. However, they do not include such falsy behavior.

In [2]: bool(datetime.datetime(datetime.MINYEAR, 1, 1))
Out[2]: True
In [3]: bool(datetime.date(datetime.MINYEAR, 1, 1))
Out[3]: True

Why don't these classes have any sense of zero at their minimums?

datetime.time.__bool__ should be dropped if for nothing more than consistency.

@python-dev
Copy link
Mannequin

@python-dev python-dev mannequin commented Mar 20, 2014

New changeset 89aa669dcc61 by Benjamin Peterson in branch 'default':
remove the ability of datetime.time to be considered false (closes bpo-13936)
http://hg.python.org/cpython/rev/89aa669dcc61

@python-dev python-dev mannequin closed this as completed Mar 20, 2014
@ahmedsayeed1982 ahmedsayeed1982 mannequin added expert-tkinter 3.8 and removed stdlib labels Nov 4, 2021
@eryksun eryksun added stdlib and removed expert-tkinter labels Nov 4, 2021
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.8 stdlib type-feature
Projects
None yet
Development

No branches or pull requests