8000 Add default tzinfo by pganssle · Pull Request #475 · dateutil/dateutil · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
8000

Add default tzinfo #475

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

Merged
merged 2 commits into from
Oct 26, 2017
Merged

Add default tzinfo #475

merged 2 commits into from
Oct 26, 2017

Conversation

pganssle
Copy link
Member

As a partial and possibly more general fix to #94, this adds a tzinfo to any datetime that doesn't have one, and otherwise leaves it alone.

@pganssle pganssle added this to the 2.7.0 milestone Oct 14, 2017
@pganssle
Copy link
Member Author

Ping @nealmcb

@nealmcb
Copy link
nealmcb commented Oct 14, 2017

Thanks for working on this!

So my proposal was implementing something like this:

>>> parse("00:00", defaulttz=tzutc())

And it seems that this would be the proposed implementation from this PR a6aae67:

>>> from dateutil.utils import default_tzinfo
>>> default_tzinfo(parse("0:00"), tzutc())

I guess that works, and if people get their date objects from somewhere other than parse, it is indeed more general. But it is less discoverable for people looking at the parse documentation. Would it make sense to put an example there?

@nealmcb
Copy link
nealmcb commented Oct 14, 2017

I guess a practical example would be something that takes two strings to be parsed from some list of meeting times, in which one string has timezone info, and the other doesn't. Both could be parsed, with a default of the NYC timezone provided, and the results printed out.

"""

dt = datetime.now(tzinfo)
return datetime.combine(dt.date(), time(0, tzinfo=tzinfo))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for today(tzinfo), but I'm very wary of implementing behavior that differs from same-named stdlib behavior. btw:

>>> tz = pytz.timezone('US/Pacific')
>>> pd.Timestamp.today(tz)
Timestamp('2017-10-14 17:19:04.081315-0700', tz='US/Pacific')

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbrockmendel Sorry, I miscalculated what would be shown in the diff here, this discussion should be taking place on #474, so I've responded here

if dt.tzinfo is not None:
return dt
else:
return dt.replace(tzinfo=tzinfo)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

from dateutil import utils
from dateutil.utils import within_delta

from freezegun import freeze_time
Copy link
Contributor
@jbrockmendel jbrockmendel Oct 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Now if I can figure out #463 to mock tzlocal, we'll make a mockery of this whole endeavor.

@pganssle
Copy link
Member Author

@nealmcb Yes, that's not a bad idea. After we settle on the final interface here, I can add it in to the documentation for the parser.

@pganssle
Copy link
Member Author

@nealmcb Added an example in the default_tzinfo documentation. Look good?

@jbrockmendel
Copy link
Contributor

LGTM

@pganssle pganssle merged commit 6932824 into dateutil:master Oct 26, 2017
@pganssle pganssle mentioned this pull request Mar 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0