Ensure now() respects DST by using .fold attribute#416
Ensure now() respects DST by using .fold attribute#416tomage wants to merge 1 commit intopython-pendulum:masterfrom
now() respects DST by using .fold attribute#416Conversation
|
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. |
|
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. |
66268ae to
5ca9195
Compare
| pygments = "^2.2" | ||
| markdown-include = "^0.5.1" | ||
| freezegun = "^0.3.12" | ||
|
|
There was a problem hiding this comment.
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.
ed9a6d4 to
1ba5fee
Compare
1ba5fee to
8098700
Compare
|
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. |
|
Thanks for your contribution! I went with a simpler fix in this PR #483. Basically, we don't need to go through |
Makes sense! That PR looks good to me - I'll close this one. |
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 callingpendulum.now('UTC').in_tz(my_timezone). Reported issue here: #417.After poking around, it seemed to me that
pendulum.datetime()function was defaulting toPOST_TRANSITIONasdst_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 thependulum.instance()method, instead ofpendulum.now().Well, looking forward for your feedback!