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

Comparing datetime.time objects incorrect for TZ aware and unaware #82993

Open
mikessut mannequin opened this issue Nov 15, 2019 · 5 comments
Open

Comparing datetime.time objects incorrect for TZ aware and unaware #82993

mikessut mannequin opened this issue Nov 15, 2019 · 5 comments
Labels
3.9 only security fixes stdlib Python modules in the Lib dir

Comments

@mikessut
Copy link
Mannequin

mikessut mannequin commented Nov 15, 2019

BPO 38812
Nosy @jsnklln, @pganssle, @mikessut

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 = None
created_at = <Date 2019-11-15.13:30:11.516>
labels = ['library', '3.9']
title = 'Comparing datetime.time objects incorrect for TZ aware and unaware'
updated_at = <Date 2019-11-18.16:44:25.729>
user = 'https://github.com/mikessut'

bugs.python.org fields:

activity = <Date 2019-11-18.16:44:25.729>
actor = 'Jason.Killen'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2019-11-15.13:30:11.516>
creator = 'epicadv'
dependencies = []
files = []
hgrepos = []
issue_num = 38812
keywords = []
message_count = 5.0
messages = ['356673', '356706', '356806', '356814', '356881']
nosy_count = 3.0
nosy_names = ['Jason.Killen', 'p-ganssle', 'epicadv']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue38812'
versions = ['Python 3.9']

@mikessut
Copy link
Mannequin Author

mikessut mannequin commented Nov 15, 2019

import pytz
import datetime

tzaware_time1 = datetime.time(7,30,tzinfo=pytz.timezone("America/Denver"))
tzaware_time2 = datetime.time(7,30,tzinfo=pytz.utc)

tzunaware_time = datetime.time(7, 30)

# This fails with exception: TypeError: can't compare offset-naive and offset-aware times
# even though both ARE tz aware.
tzaware_time1 < tzaware_time2

# This does NOT raise an exception and should since one is aware and one isn't.
tzunaware_time < tzaware_time1

@jsnklln
Copy link
Mannequin

jsnklln mannequin commented Nov 15, 2019

This appears to be a bug in pytz which as far as I can tell is not part of the "standard" lib, at least it's not in Lib.

Running this example which does not use pytz:
from datetime import timedelta, datetime, tzinfo, timezone, time

# stole this from the docs, and beat it up a bit
class KabulTz(tzinfo):
    def utcoffset(self, dt):
        return timedelta(hours=4, minutes=30)

    def fromutc(self, dt):
        # Follow same validations as in datetime.tzinfo
        if not isinstance(dt, datetime):
            raise TypeError("fromutc() requires a datetime argument")
        if dt.tzinfo is not self:
            raise ValueError("dt.tzinfo is not self")
    return dt + timedelta(hours=4)
    def dst(self, dt):
        # Kabul does not observe daylight saving time.
        return timedelta(0)

    def tzname(self, dt):
        return "+04"

tzaware_time2 = time(7,30,tzinfo=timezone.utc)
tzaware_time1 = time(7,30,tzinfo=KabulTz())

tzunaware_time = time(7, 30)

tzaware_time1 < tzaware_time2

I get no error.

When I use your example I notice that utcoffset in pytz/tzinfo.py returns None for the Denver example but for the UTC object returns ZERO which happens to be timedelta(0) which seems correct. It's weird to me that pytz is returning None in the Denver case because I think what it should return is right there in self._utcoffset.

More info: It looks like pytz isn't handling the fact that it's a time only type. dt appears to be a datetime when utcoffset is called from a datetime.

@mikessut
Copy link
Mannequin Author

mikessut mannequin commented Nov 17, 2019

Ok. I'll file a bug on pytz. Thanks!

On Sat, Nov 16, 2019 at 11:18 PM Karthikeyan Singaravelan <
report@bugs.python.org> wrote:

Change by Karthikeyan Singaravelan <tir.karthi@gmail.com>:

----------
nosy: +p-ganssle


Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue38812\>


@pganssle
Copy link
Member

I do not think this is a bug in pytz, but if it's a bug in Python it's one in reporting what the error is.

The issue is that the time zone offset for "rules-based zones" like America/Denver (i.e. most time zones) is *undefined* for bare times, because the offset that apply depends on the *date* and the *time*.

The documentation for tzinfo.utcoffset specifies that if the offset is unknown, a time zone offset should return None: https://docs.python.org/3/library/datetime.html#datetime.tzinfo.utcoffset

The documentation for determining whether an object is aware or naive also specifies that if utcoffset() returns None, the object is naive (even if tzinfo is not None): https://docs.python.org/3/library/datetime.html#determining-if-an-object-is-aware-or-naive

So basically, everyone is doing the right thing except the person who attached this pytz time zone to a time object (as a side note, it may be worth reading this blog post that explains why the way this time zone is attached to the time object is incorrect: https://blog.ganssle.io/articles/2018/03/pytz-fastest-footgun.html).

That said, we may be able to improve the error message raised here by distinguishing between the case where there's no tzinfo at all and the case where utcoffset() returns None. I think we can change the exception message to have a helpful hint like, "cannot compare offset-naive and offset-aware times; one of the operands is offset-naive because its offset is undefined."

We could possibly be even more specific.

@pganssle pganssle added stdlib Python modules in the Lib dir 3.9 only security fixes labels Nov 17, 2019
@jsnklln
Copy link
Mannequin

jsnklln mannequin commented Nov 18, 2019

Yep I wasn't seeing the forest for the trees. Thanks for clearing that up. I'll swoop back in and see what I can do.

@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.9 only security fixes stdlib Python modules in the Lib dir
Projects
Development

No branches or pull requests

1 participant