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

Ensure now() respects DST by using .fold attribute #416

Conversation

tomage
Copy link
Contributor

@tomage tomage commented Oct 17, 2019

I found that calling pendulum.now(my_timezone) for a timezone that has DST, right after DST was turned off gave me different results over calling pendulum.now('UTC').in_tz(my_timezone). Reported issue here: #417.

After poking around, it seemed to me that pendulum.datetime() function was defaulting to POST_TRANSITION as dst_rule, which was the path followed when calling .now() on the DST sensitive timezone, whereas calling .now('UTC') and then following that with ._in_tz(my_timezone) did indeed do something different - it had a bit of code to figure out the transition to apply.

So - perhaps it's a bit crude, but I pretty much copied that logic over, and ran the test suite. It also seemed a bit more symmetrical, code-wise. It's likely that there's some refactoring (i.e. extract that logic out) that could be done, but I didn't want to go too far, as I'm pretty new to the library.

I then added 4 tests to make sure to cover all possible points in time w.r.t. DST transitions - before, right after turning on, during and right after turning off.

The one thing I wasn't sure about was the usage of freezegun. It's a pretty popular library however, and I've battle tested it myself quite a bit. I've also compared it to libfreezetime-which I like--but freezegun has way less overhead. I also compared it to simply changing system time.

Feel free to push back on bringing in freezegun however. I did try to mock.patch the call to _datetime.datetime.now(), but built-ins can't be patched of course. So freezegun just seemed like the sanest way to go about this. Unless I move the unit-tests to test the pendulum.instance() method, instead of pendulum.now().

Well, looking forward for your feedback!

@tomage
Copy link
Contributor Author

tomage commented Oct 17, 2019

Hmh. Didn't think of other python versions. Of course .fold doesn't exist for python<=3.5.

This will have to be mended of course, but I think I'll wait for feedback from you, sdispater, before spending more time on this. Perhaps there's a more obvious solution to the problem I'm not seeing.

@tomage
Copy link
Contributor Author

tomage commented Oct 17, 2019

Actually - went for a quick-n-dirty fix - tho it's not fixing pypy3. Haven't used pypy myself, but may give it a go later.

@tomage tomage force-pushed the fix-now-in-tz-after-dst-transition branch from 66268ae to 5ca9195 Compare March 8, 2020 03:22
@@ -41,6 +41,8 @@ mkdocs = { version = "^1.0", python = "^3.5" }
pymdown-extensions = "^6.0"
pygments = "^2.2"
markdown-include = "^0.5.1"
freezegun = "^0.3.12"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So - before adding this, I noticed that the poetry.lock file was already out of date, so I didn't want to muck around with it (yet?).
So for now, just adding this dependency here in the pyproject.toml file.

Now.. I did actually have some issues adding this using poetry (latest version, installed via curling that install script into bash). Tho as far as I could see, I had the correct version of python (3.6.5). But it was complaining about black vs. python interoperability... At any rate - I figured you might want to straighten out poetry.lock on master anyway.

@tomage tomage force-pushed the fix-now-in-tz-after-dst-transition branch 3 times, most recently from ed9a6d4 to 1ba5fee Compare July 5, 2020 23:41
@tomage tomage force-pushed the fix-now-in-tz-after-dst-transition branch from 1ba5fee to 8098700 Compare July 10, 2020 08:37
@tomage
Copy link
Contributor Author

tomage commented Jul 10, 2020

I'm not married to the idea of using freezegun - there might easily be a nicer way to test this.. But feel free to take this one over @sdispater, but I'm also open up to all/any suggestions.

@sdispater
Copy link
Collaborator

Thanks for your contribution!

I went with a simpler fix in this PR #483.

Basically, we don't need to go through instance() in now() and we can return a DateTime directly.

@tomage
Copy link
Contributor Author

tomage commented Jul 12, 2020

Thanks for your contribution!

I went with a simpler fix in this PR #483.

Basically, we don't need to go through instance() in now() and we can return a DateTime directly.

Makes sense! That PR looks good to me - I'll close this one.

@tomage tomage closed this Jul 12, 2020
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.

2 participants