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

Add tz kwarg to from_timestamp() #1621

Merged
merged 8 commits into from
May 1, 2020
Merged

Add tz kwarg to from_timestamp() #1621

merged 8 commits into from
May 1, 2020

Conversation

Bibo-Joshi
Copy link
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
@plammens
Copy link
Contributor

plammens commented Nov 18, 2019

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
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
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
Member

agree with the 2.7 part

tests/test_helpers.py Outdated Show resolved Hide resolved
tests/test_helpers.py Outdated Show resolved Hide resolved
telegram/utils/helpers.py Outdated Show resolved Hide resolved
tests/test_helpers.py Show resolved Hide resolved
@Bibo-Joshi
Copy link
Member Author

@plammens Lost in translation … Thanks for clarifying!

Copy link
Member

@tsnoam tsnoam left a comment

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.

telegram/utils/helpers.py Outdated Show resolved Hide resolved
@tsnoam
Copy link
Member

tsnoam commented May 1, 2020

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants