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

make datetime objs timezone-aware and UTC default #256

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

sarnold
Copy link

@sarnold sarnold commented Nov 11, 2018

  • requires (assumes) standard config of local system and hw clocks
    (everything is UTC by default using system TZ offset)

After looking through several timezone-related PRs/issues, I think a simple UTC time config is probably the safest default, although it does rely on the system timezone being set. That said, current Linux distros, modern Linux kernels, NTP, etc, are pretty standard. Use as you see fit.

@coveralls
Copy link

coveralls commented Nov 11, 2018

Coverage Status

Coverage decreased (-6.4%) to 93.56% when pulling fa01b4b on sarnold:utc-time into 41f88b3 on dbader:master.

@sarnold
Copy link
Author

sarnold commented Nov 12, 2018

Hmm, the tests run here fine, and travis seems happy except for 2.7 where it bails with an import error building docs. Got any tips for that?

@SJLC
Copy link

SJLC commented Nov 12, 2018

If you're going to always use utc for timezone, how about datetime.datetime.utcnow() in place of datetime.datetime.now(timezone.utc)

* fix mock now() and tests with a timezone
* allow flake8-max-line-length to be 90
* fixes for lack of datetime.timezone in python 2.7 (simple UTC)

Signed-off-by: Steve Arnold <[email protected]>
@sarnold
Copy link
Author

sarnold commented Nov 13, 2018

I think I'm pretty much done with this one (pending feedback). Thanks for making me update/add tests and learn something new... ;)

* requires (assumes) standard config of local system and hw clocks
  (everything is UTC by default using system TZ offset)

Signed-off-by: Steve Arnold <[email protected]>
* for getting logging from the parent package, it needs to specify a
  logfile/path in logging.basicConfig, import logging, etc

Signed-off-by: Steve Arnold <[email protected]>
* rebase of original patch over update-logging patch

Signed-off-by: Steve Arnold <[email protected]>
* use this to enable separate schedule log from parent application
  "from schedule.parent_logger import setup_logging"

Signed-off-by: Steve Arnold <[email protected]>
* last_run and idle_seconds_since are both None until jobs run once

Signed-off-by: Steve Arnold <[email protected]>
Testing 051 pre-release looks good so far; tag for release and shipping on dev...
* requires (assumes) standard config of local system and hw clocks
  (everything is UTC by default using system TZ offset)

Signed-off-by: Steve Arnold <[email protected]>
Testing 051 pre-release looks good so far; tag for release and shipping on dev...
sarnold and others added 13 commits November 14, 2019 13:43
@sarnold
Copy link
Author

sarnold commented Nov 14, 2019

Hmm, this looks okay now, albeit with some extra functionality and reworked tests. Let me know if you'd rather have some squashing and/or more separation.

@sarnold
Copy link
Author

sarnold commented Dec 22, 2019

Note this is no longer just the timezone-awareness after the recent rebase; it now includes a few extra "features" like a tag parameter and some new job properties. Also includes new tests for the above and green builds on linux/os/x and all the python versions.

@sarnold
Copy link
Author

sarnold commented Dec 22, 2019

Current totals are:

----------- coverage: platform linux, python 3.7.1-final-0 -----------
Name                   Stmts   Miss  Cover   Missing
----------------------------------------------------
schedule/__init__.py     274      4    99%   42-43, 462-466

26 passed in 0.93 seconds =====================

versus:

----------- coverage: platform linux, python 3.7.1-final-0 -----------
Name                        Stmts   Miss  Cover   Missing
---------------------------------------------------------
schedule/__init__.py          304      7    98%   42-43, 54-56, 508-512
schedule/parent_logger.py       8      0   100%
schedule/timezone.py           12      3    75%   13, 16, 19
---------------------------------------------------------
TOTAL                         324     10    97%

35 passed in 1.17s ==========================

@kmcminn
Copy link

kmcminn commented Feb 12, 2020

came here because I was looking for an explicit way to define the timezone.
as a user I would like to specify the option (explicit timezone or server timezone).

couple nits:

  • use pytz instead of timezone.utc (more sustainable, also to support user defined timezone)
  • datetime.datetime.now(timezone.utc) is the correct way to get utc in python (utcnow() is naive)

besides this it lgtm

@gogognome
Copy link

When will this branch be merged? We need it because it probably fixes the problem we had during the ending of Daylight Saving Time: we schedule jobs to run every minute, but for one hour no jobs are executed because the local time is changed from 3:00 to 2:00.

@samgermain
Copy link

If you're going to always use utc for timezone, how about datetime.datetime.utcnow() in place of datetime.datetime.now(timezone.utc)

datetime.utcnow() is suppossed to be worse than datetime.now(timezone.utc)

@samgermain
Copy link

Yeah can you merge this? I'm trying to schedule something, and can convert the time so that it matches the correct utc time, but I think daylight savings will mess it up

@sarnold
Copy link
Author

sarnold commented Oct 20, 2021

My new PR #489 is an attempted re-work of this one; if someone could test it, that would be super-helpful. Note my particular need (at the time) was reasonable UTC log times for long-running/daemonized scheduler. Hopefully that still fits someone else's need, otherwise the answer is basically a fully-tzaware scheduler with default/per-job tz attributes (give or take the logging setup).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants