Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Fix UTC/local inconsistencies for naive datetimes#1506

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
tsnoam merged 14 commits intopython-telegram-bot:masterfromplammens:fix-datetimes
Nov 15, 2019

Conversation

plammens
Copy link
Contributor

Closes#1505.

(cherry-picked fromc6dd3d7.I've included the refactoring mentioned inpython-telegram-bot#1497 to facilitate thechange.)There was inconsistent use of UTC vs local times. For instance, in theformer `_timestamp` helper (now `_datetime_to_float_timestamp`), assumedthat naive `datetime.datetime` objects were in the local timezone, whilethe `from_timestamp` helper —which I would have thought was thecorresponding inverse function— returned naïve objects in UTC.This meant that, for instance, `telegram.Message` objects' `date` fieldwas constructed as a naïve `datetime.datetime` (from the timestamp sentby Telegram's server) in *UTC*, but when it was stored in `JSON` formatthrough the `to_json` method, the naïve `date` would be assumed to be in*local time*, thus generating a different timestamp from the one it wasbuilt from.Seepython-telegram-bot#1505 for extended discussion.
Some tests/test fixtures that were using `datetime.datetime.now()` asa test value, were changed to `datetime.datetime.utcnow()`, since noweverything is (hopefully) expecting UTC for naive datetimes.
@Bibo-JoshiBibo-Joshi added the 📋 pending-reviewwork status: pending-review labelSep 6, 2019
@PoolitzerPoolitzer added this to the12.2 milestoneSep 15, 2019
Copy link
Member

@jh0kerjh0ker left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. I left two comments, if you could address them that would be great.

@jh0kerjh0ker added 📋 pending-replywork status: pending-reply and removed 📋 pending-reviewwork status: pending-review labelsOct 11, 2019
@jh0ker
Copy link
Member

Some more questions that popped up during the discussion in the dev group. Are timezone-aware datetimes in the job queue supported with this? Were they supported before? There seem to be no tests for this yet.

@jh0kerjh0ker added the 📋 pending-reviewwork status: pending-review labelOct 13, 2019
@plammens
Copy link
ContributorAuthor

Are timezone-aware datetimes in the job queue supported with this?

Yes; namely,when inrun_once,first inrun_repeating, andtime inrun_daily now all accept timezone-aware objects (and use that information adequately). It all boils down to_datetime_to_float_timestamp, which now supports aware datetimes, so I believe everything else in the repository that handles date/time objects by converting them to timestamps also supports aware datetimes now.

https://github.com/plammens/python-telegram-bot/blob/56466ac449028e4c5eb0470c919ec2b7466d6737/telegram/utils/helpers.py#L58-L61

Were they supported before?

No; for example you would get aTypeError when passing an aware datetime towhen inrun_once.

There seem to be no tests for this yet.

Will add in the near future.

jh0ker reacted with heart emoji

@jh0kerjh0ker removed the 📋 pending-replywork status: pending-reply labelOct 13, 2019
@jh0kerjh0ker requested a review fromtsnoamOctober 13, 2019 22:39
@jh0ker
Copy link
Member

Thank you very much! This looks very nice to me, but timezones confuse me so i requested a review from@tsnoam 😉

Copy link
Member

@tsnoamtsnoam left a comment

Choose a reason for hiding this comment

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

All in all, this is really great. Thank you!

However, I have several comments / changes needs to be done.
Will you be so kind to check them out?

@tsnoamtsnoam added 📋 pending-replywork status: pending-reply and removed 📋 pending-reviewwork status: pending-review labelsOct 14, 2019
@plammens
Copy link
ContributorAuthor

plammens commentedOct 18, 2019•
edited
Loading

@tsnoam Before merging I think I should add:

  • Tests to check that timezone aware datetimes work
  • A general note somewhere (where?) in the documentation stating that all naive datetimes will be assumed to be in UTC
  • Afrom_float_timestamp for symmetry? Or renamefrom_timestamp tomaybe_from_timestamp to indicate that is just a wrapper overdatetime.datetime.fromtimestamp?
  • Add capability for aware datetimes tofrom_timestamp?
  • Restrictto_float_timestamp to non-None times

Anything else/any thoughts?

Copy link
Member

@tsnoamtsnoam left a comment

Choose a reason for hiding this comment

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

Hi@plammens

This looks very good. Almost ready for merge :-)

  1. About the tests: yes, we need tests... also the tests@jh0ker had suggested in his review.
  2. I don't think any extra documentation is required for now. The docstring ofto_float_timestamp is good enough.
  3. Re: renaming funciton names - see the comments on the review notes. But basically, leave it as is.
  4. Still need to fixJobQueue._put() so it won't passNone toto_float_timestamp().
  5. Adding similar capabilities tofrom_timestamp() can be done in a different PR. Lets wrap this one and merge it already ;-)

@plammens
Copy link
ContributorAuthor

plammens commentedNov 11, 2019•
edited
Loading

@tsnoam I've addressed the remaining points and added tests.

Sorry for the delay!

A job shouldn't (and can't) be enqueued with `next_t = None`. Anexception should be raised at `_put` before an obscure error occurslater down the line.
@tsnoam
Copy link
Member

tsnoam commentedNov 15, 2019•
edited
Loading

@plammens
Thank you very much for your contribution. Also your patience for us to reply and with all our requests is highly appreciated.
I've pushed a minor cosmetic fix, and now we're just waiting for unitests to pass (no reason they shouldn't) before merging.

plammens reacted with heart emoji

@tsnoam
Copy link
Member

I don't know why pre-commit test failed. On my machine it passes successfully.
Merging anyway.

@tsnoamtsnoam merged commit4e717a1 intopython-telegram-bot:masterNov 15, 2019
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsAug 19, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@jh0kerjh0kerjh0ker left review comments

@tsnoamtsnoamtsnoam approved these changes

Assignees
No one assigned
Labels
📋 pending-replywork status: pending-reply
Projects
None yet
Milestone
12.3
Development

Successfully merging this pull request may close these issues.

Inconsistent use of UTC/local naïve datetimes
5 participants
@plammens@jh0ker@tsnoam@Bibo-Joshi@Poolitzer
[8]ページ先頭
©2009-2025 Movatter.jp