Fix issue #150 implementing an Interval class #154
Conversation
This is a POC, targeting issue MagicStack#150: PostgreSQL interval data type and Python timedelta have a similar goal, but are subtly different because PG date arithmetic is much more versatile. So, instead of trying to coerce an interval to a timedelta, the new class aims to carry the same bits of information, and may implement a similar semantic, if needed.
Implement most of the PostgreSQL interval arithmetic, making the new class a credible replacement of the Python timedelta.
This seems more natural and allows Sphinx to extract its documentation.
| | ||
| return result + delta | ||
| | ||
| elif isinstance(self, datetime.date): |
There was a problem hiding this comment.
How can this branch be ever triggered? self should always be an instance of Interval, doesn't it?
There was a problem hiding this comment.
| return ((self.months, self.days, self.time) | ||
| >= | ||
| (other.months, other.days, other.time)) | ||
| return False |
There was a problem hiding this comment.
Should we return NotImplemented here? Can we add a test for this?
There was a problem hiding this comment.
Here too, from my understanding and tests of the Cython richcmp(), if other is not an instance of Interval then it should return False. I will try to find again the examples that lead me there.
| Should we add an |
| readonly int64_t time | ||
| | ||
| def __init__(self, int32_t months, int32_t days, int64_t time=0, | ||
| int64_t hours=0, int64_t minutes=0, int64_t seconds=0, int64_t microseconds=0): |
There was a problem hiding this comment.
All code lines must be shorter than 79 chars.
There was a problem hiding this comment.
Ok, will rectify that.
| self.days - other.days, | ||
| self.time - other.time) | ||
| | ||
| elif isinstance(self, datetime.date): |
There was a problem hiding this comment.
Again, I'm not sure how this branch can ever be triggered.
| This change can also break backwards compatibility. Any code that expected to work with |
I thought about that, but they are really not compatible, at least when they contain months: there's no obvious way to represent a PG interval with a Python timedelta (... otherwise we could avoid the new Interval class all together). |
Yeah, I know. But I still think that is less surprising having a different instance, than a wrong value as the original issue demonstrate, isn't it? |
| Thank you for the review! |
| After some consideration with @1st1 we decided that merging a custom
|
| Thanks a lot for finding and taking the time to tackle the issue, @lelit! |
| Ok, up to you. I will then try to implement this as a third party module. |
3 participants
See issue #150 for details.