Skip to content

Add tz kwarg to from_timestamp()#1621

Merged
Bibo-Joshi merged 8 commits into
masterfrom
tz_from_timestamp
May 1, 2020
Merged

Add tz kwarg to from_timestamp()#1621
Bibo-Joshi merged 8 commits into
masterfrom
tz_from_timestamp

Conversation

@Bibo-Joshi

Copy link
Copy Markdown
Member

Closing #1613 as addendum to #1506

@tsnoam @plammens Is this, what was missing or am I overlooking half of it? :D

@Bibo-Joshi Bibo-Joshi added this to the 12.5 milestone Nov 18, 2019
@Bibo-Joshi Bibo-Joshi requested a review from tsnoam November 18, 2019 19:37
@plammens

plammens commented Nov 18, 2019

Copy link
Copy Markdown
Contributor

With this the default is an aware datetime, while before it was naive—e.g. telegram.Message.date will return an aware datetime. If we want Message.date and the like to keep being naive, the calls in the de_json of some telegram classes, e.g.

data['date'] = from_timestamp(data['date'])
would also need to be changed to data['date'] = from_timestamp(data['date']).replace(tzinfo=None). Or the default for from_timestamp could be kept as naive UTC to avoid the breaking change.

Otherwise I think the only problem is handling None: passing None to tzinfo will return a naive datetime in local rather than UTC. Maybe something like this instead:

if tzinfo is not None:
    return dtm.datetime.fromtimestamp(unixtime, tz=tzinfo)
else:
    return dtm.datetime.utcfromtimestamp(unixtime)

@Bibo-Joshi

Copy link
Copy Markdown
Member Author

With this the default is an aware datetime, while before it was naive—e.g. telegram.Message.date will return an aware datetime. If we want Message.date and the like to keep being naive, the calls in the de_json of some telegram classes, e.g.

data['date'] = from_timestamp(data['date'])

would also need to be changed to data['date'] = from_timestamp(data['date']).replace(tzinfo=None). Or the default for from_timestamp could be kept as naive UTC to avoid the breaking change.

Good point. Let's wait, what tsoam says.

Otherwise I think the only problem is handling None: passing None to tzinfo will return a naive datetime in local rather than UTC. Maybe something like this instead:

if tzinfo is not None:
    return dtm.datetime.fromtimestamp(unixtime, tz=tzinfo)
else:
    return dtm.datetime.utcfromtimestamp(unixtime)

Oh, shuh, you're absolutely right. Will update tonight.

@Bibo-Joshi

Copy link
Copy Markdown
Member Author

Updated as suggested. About the failing tests:

  • Py2.7 fails because datetime has no timezone. But since this PR is only due for v12.5, which I reckon won't be released bevore 2020, when we drop Py2.7, I guess we don't really have to adjust for that. Although, I wonder why it didn't fail on the first run …
  • As usual, I have no clue, what codecov wants from me 🤔

@Poolitzer

Copy link
Copy Markdown
Member

agree with the 2.7 part

Comment thread tests/test_helpers.py Outdated
Comment thread tests/test_helpers.py Outdated
Comment thread telegram/utils/helpers.py Outdated
Comment thread tests/test_helpers.py
@Bibo-Joshi

Copy link
Copy Markdown
Member Author

@plammens Lost in translation … Thanks for clarifying!

@Bibo-Joshi Bibo-Joshi added the 📋 pending-review work status: pending-review label Nov 23, 2019

@tsnoam tsnoam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

one minor comment on a docstring. but LGTM, you can merge.

Comment thread telegram/utils/helpers.py Outdated
@tsnoam

tsnoam commented May 1, 2020

Copy link
Copy Markdown
Member

actually, i just fixed the docstring myself.

you can merge from master and then you can merge this PR.

# Conflicts:
#	tests/conftest.py
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

📋 pending-review work status: pending-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants