-
Notifications
You must be signed in to change notification settings - Fork 7
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
Allow passing single date/datetime to postgres db range fields #150
Conversation
e96de12
to
79259c8
Compare
tests/storage/test_storage.py
Outdated
@@ -1226,7 +1226,7 @@ def test_query_dataframe_with_sql(self): | |||
assert result_df.shape == (1, 2) | |||
|
|||
assert result_df["string_column"][0] == "A" | |||
assert result_df["array_column"][0] == [1, 2, 3] | |||
assert tuple(result_df["array_column"][0]) == (1, 2, 3,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this test was failing, and this isn't a fix that suggests any understanding, but it does work.
cb998ea
to
3422f53
Compare
3422f53
to
c0194b1
Compare
c0194b1
to
4ebdec0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff! I have a couple of minor comments but it looks to be mostly in order.
with pytest.raises( | ||
TypeError, | ||
match="HalfFiniteDateTimeRangeField may only accept HalfFiniteDateTimeRangeField or datetime objects", | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐼 The use of match=
is a good addition but perhaps it warrants a separate commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I'll split this change out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return pg_ranges.DateRange(lower=value.start, upper=value.end, bounds="[]") | ||
if ( | ||
isinstance(value, datetime.date) | ||
# Don't allow datetime (datetime is a subclass of date!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has always bugged me about the datetime library 😐 .
@@ -158,22 +166,18 @@ class HalfFiniteDateTimeRangeField(pg_fields.DateTimeRangeField): | |||
""" | |||
|
|||
def get_prep_value( | |||
self, value: Optional[ranges.HalfFiniteDatetimeRange] | |||
) -> Optional[pg_ranges.DateTimeTZRange]: | |||
self, value: Any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Any
here and not Optional[Union[ranges.HalfFiniteDatetimeRange, datetime.datetime]]
? (And similar for the other get_prep_value
implementations).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By my understanding, Any
will be the correct type hint here. We can't enforce that the value will be Optional[Union[ranges.HalfFiniteDatetimeRange, datetime.datetime]]
, since there is nothing stopping us doing things like MyModel.objects.filter(period__contains="foo")
(or any other arbitrary type). This type checking happens at runtime (hence the raise TypeError
), not mypy.
By using match we can be sure we are catching the exception we expect.
https://app.circleci.com/pipelines/github/octoenergy/xocto/586/workflows/9b2278c5-ea72-4d9f-862d-a7be672ce21f/jobs/571 > tests/storage/test_storage.py:1229: in test_query_dataframe_with_sql > assert result_df["array_column"][0] == [1, 2, 3] > E ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()
4ebdec0
to
2352ce9
Compare
Fixes #149