-
Notifications
You must be signed in to change notification settings - Fork 769
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
resolve subtypes of postgres RangeField to new RangeType #677
Conversation
I have seen the tests failed for IntegerRangeField, it was working as List type. |
I think it would be good to find examples of ranges being used currently in GraphQL. Unfortunately I couldn't find any. Here's another graphql project looking to support ranges, but no specifics have been defined: hasura/graphql-engine#1082 |
@@ -1,6 +1,6 @@ | |||
from .types import DjangoObjectType | |||
from .fields import DjangoConnectionField | |||
|
|||
__version__ = "2.3.0" | |||
__version__ = "2.3.2" |
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.
Can you revert this? The version number will be incremented when we create a new release.
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'll check. I didn't remember I changed version, could it be due to merge from some branch?
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'm not that familiar with the Range types in postgres but this approach seems ok. This PR needs some tests though.
|
||
class DateTimeRangeType(RangeResolver, ObjectType): | ||
lower = DateTime() | ||
upper = DateTime() |
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.
Are these fields required
?
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.
Not quite, I think it could be optional.
One problem is the original method which returns a tuple for (lower, upper), it is working for IntegerRange which is tested in test cases, but it is not working for DateTimeRange.
def resolve_upper(parent, info): | ||
return parent.upper | ||
|
||
class DateTimeRangeType(RangeResolver, ObjectType): |
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.
According to http://initd.org/psycopg/docs/extras.html#adapt-range bounds
and empty
fields are missing.
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.
bounds is not accessible. you said missing is referring to mutation?
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.
@jkimbo here is attributes:
Object attributes:
isempty
True if the range is empty.
lower
The lower bound of the range. None if empty or unbound.
upper
The upper bound of the range. None if empty or unbound.
lower_inc
True if the lower bound is included in the range.
upper_inc
True if the upper bound is included in the range.
lower_inf
True if the range doesn’t have a lower bound.
upper_inf
True if the range doesn’t have an upper bound.
I see original method either not implement it, so I didn't too.
def resolve_lower(parent, info): | ||
return parent.lower | ||
def resolve_upper(parent, info): | ||
return parent.upper |
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.
If the resolvers just pick attributes on the parent object that are the same name as the field then you don't need these resolvers.
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'll try
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Original implementation to resolve Django.contrib.postgres Range series fields to a List type,
which is not working for DateTimeTZRange ( at least for Django 2.2.x )
#675
What I did is to resolve Range fields to specific RangeType with upper and lower fields inside it, like