Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd issubset, issuperset and size to Range type #563
Conversation
| lowerOk = True | ||
| else: | ||
| if self._lower is None: | ||
| return False |
elprans
Apr 24, 2020
Member
It would read better if you set lowerOk = False here and use an elif. Also, please use lower_ok for the variable name for style consistency.
It would read better if you set lowerOk = False here and use an elif. Also, please use lower_ok for the variable name for style consistency.
| def issuperset(self, other): | ||
| return other.issubset(self) | ||
|
|
||
| def size(self, step=None): |
elprans
Apr 24, 2020
Member
I do not understand why the step argument is necessary. The size of the range is determined unambiguously by whether the upper and lower bounds are inclusive.
I do not understand why the step argument is necessary. The size of the range is determined unambiguously by whether the upper and lower bounds are inclusive.
kdorsel
Apr 24, 2020
•
Author
The returned size for [1,2) and [1,2] should be different. In postgres they have a distinction between a discrete and continuous range. If this was a discrete integer range the results would be 2-1 = 1 and 2-1+1=2. +1 since the discrete step for an integer is 1. What is the step value for continuous range like a float? Or if using datetime what is your lowest time increment you care about.
The returned size for [1,2) and [1,2] should be different. In postgres they have a distinction between a discrete and continuous range. If this was a discrete integer range the results would be 2-1 = 1 and 2-1+1=2. +1 since the discrete step for an integer is 1. What is the step value for continuous range like a float? Or if using datetime what is your lowest time increment you care about.
elprans
Apr 24, 2020
Member
OK, I think the generic Range class shouldn't be implementing the size() method then. It's too dependent on the data type and, since Postgres allows user-defined range types, hardcoding it for int, float, and date types wouldn't save us.
OK, I think the generic Range class shouldn't be implementing the size() method then. It's too dependent on the data type and, since Postgres allows user-defined range types, hardcoding it for int, float, and date types wouldn't save us.
|
|
||
| def size(self, step=None): | ||
| if self._upper is None or self._lower is None: | ||
| return None |
elprans
Apr 24, 2020
Member
It would be better to raise a ValueError here. size() on an open range is meaningless and akin to division by zero, returning None in this case risks masking bugs.
It would be better to raise a ValueError here. size() on an open range is meaningless and akin to division by zero, returning None in this case risks masking bugs.
kdorsel
Apr 24, 2020
•
Author
As I understood it, a None in either lower/upper means infinity which matches the null/missing bound definition of postgres. Hence why I return the "infinity". I'm ok either way.
For numbers this could be changed from None to float("inf"), and for datetimes to datetime.datetime.min/max.
As I understood it, a None in either lower/upper means infinity which matches the null/missing bound definition of postgres. Hence why I return the "infinity". I'm ok either way.
For numbers this could be changed from None to float("inf"), and for datetimes to datetime.datetime.min/max.
kdorsel
Apr 24, 2020
•
Author
Since postgres only has numeric and date/time ranges adding type check for datetime and numeric and returning values for each one and removing None completely might be worthwhile refactor.
It would also simply the logic in here since there is no longer a need for checking None.
Since postgres only has numeric and date/time ranges adding type check for datetime and numeric and returning values for each one and removing None completely might be worthwhile refactor.
It would also simply the logic in here since there is no longer a need for checking None.
| else: | ||
| upperOk = self._upper < other._upper | ||
|
|
||
| return lowerOk and upperOk |
elprans
Apr 24, 2020
Member
lowerOk is always True by this point, so returning just upperOk is sufficient.
lowerOk is always True by this point, so returning just upperOk is sufficient.
|
Looks good now. Please add some tests. |
|
Please fix flake8 issues, and let's merge. Thanks! |
Adding issubset, issuperset and size methods to the Range type.
This should correctly handle all corner cases with empty, lower/upper included or not for set methods. For empty sets follows the same rules as Python's super and sub set methods.
Size method takes in optional step parameter for when lower_inc == upper_inc since this will either add or remove 1 step size to the size of the range.
If this initial implementation is something worthwhile I can also add tests for it.
Closes #559