Fix comparison across Daylight Savings Time boundary

[esauser]

Compares datetimes using timestamps to avoid issues when comparing across the boundary when transitioning off of Daylight Savings Time. This is technically a behavior change, but only during this transition. Since it has never worked properly, submitting as a bug fix.

Pull Request Check List

• Added tests for changed code.
• Updated documentation for changed code. - No updates necessary

[NickFabry]

Hi @esauser; first, thanks for taking the time and trouble to figure out exactly why comparisons were in error, and pushing a fix!

These errors inherit directly from the stdlib python datetime module, and I believe your push would fix comparisons between DateTime objects across the DST boundary. I see how you handled the case of a comparison between something other than a DateTime and a DateTime, and that looks like it should work, or at least work as well as whatever the "other" has implemented for le, lt, etc.

However, I'm not sure if it would address the reason that the _cmp method was first created as it was, which is to assure compatibility with PyPy. I know almost nothing about PyPy, and whether it still needs the **kwargs handling or not. It was first added in commits b4a598f and be9df1d. Do you have any experience with PyPy, or knowledge about what this might be for?

Also, while the provided test does compare if pendulum.DateTimes compare correctly, I'd appreciate it if you could add some tests comparing pendulum.DateTimes to datetime.datetime instances, since at a minimum, we'd want those comparisons to be correct as well.

I'm a (very) new maintainer, so kindly explain things in maybe a little more detail than you would otherwise use - thank you.

[esauser]

@NickFabry thank you for the review. I've added that I believe to be the tests you asked for; that was a good call out. I don't love how duplicated they seem to be, but I also couldn't come up with a great way not to do that. Let me know if there was anything more there you wanted.

I, too, am in uncharted waters here. Prior to recently inheriting this project I have no Python experience. I noticed that the project was referencing using this fork and the only difference is the change I'm proposing here. I'd like to stop using the fork and point to this upstream, but to do that I need that functionality here.

Tagging @tomage as he was the original contributor on the fork. Maybe he can help with your questions.

[NickFabry]

Hi @esauser - okay, I looked as deep into PyPy as I had the time to do... and I still don't know why the _cmp function was changed the way it was. However, I also looked into the python stdlib source for datetime, and now I understand why only the _cmp function was used; the results of the _cmp function are used by all the other comparatives in datetime. See:

https://github.com/python/cpython/blob/main/Lib/datetime.py

So, it seems the most conservative thing to do is keep the _cmp function with its overrides of **kwargs, and put the correct comparison to be made there, and not add the le, lt, ge, and gt functions; something like the following:

def _cmp(self, other, **kwargs):
    # Fix for pypy which compares using this method
    # which would lead to infinite recursion if we didn't override
    kwargs = {"tzinfo": self.tz}
    if _HAS_FOLD:
        kwargs["fold"] = self.fold
    sts = self.timestamp()
    ots = other.timestamp()
    return 0 if sts == ots else 1 if sts > ots else -1

Could you amend your fix to the above for _cmp and remove the le, lt, ge and gt functions?

And the test code is longer and more gnarly, but it does what it's supposed to do - thanks for updating it thoroughly!

[esauser]

@NickFabry I am unable to pass the tests without the le, lt, ge, and gt. I tried everything I could come up with. The code never hits the _cmp. It always seems to go to the usual one, which we know does not work.

[esauser]

@NickFabry what can we do to move this forward?

[NickFabry]

Hi @esauser - sorry for the incredibly long delay - I'll have bandwidth when I get home Friday to look at your fix, and commit it. It is an important fix!

[NickFabry]

The changes look good to me. Note that the new comparison methods don't have type annotations, but I figure we can add those later, since the result of the function is more important than the type hinting.

[esauser]

pre-commit.ci autofix