-
Notifications
You must be signed in to change notification settings - Fork 16
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
57 non null fields #98
Conversation
django_graph_api/graphql/types.py
Outdated
def value(self): | ||
value = self.get_value() | ||
if value is None: | ||
return self.return_null() |
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 think I'd prefer to see this all handled in get_value()
rather than having a separate return_null method. It feels like it adds to the complexity of the object without a benefit of abstraction. I'd also rather not have a separate value
property for the same reason, but I could see an argument for it in terms of ease of use of the API.
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 would like to keep using get_value()
as the main way to access the value, but since it is being implemented in a few different models (and might be overridden in custom models), I didn't want that part to need to be implemented in each one. Do you have a suggestion for a nicer way of handling this?
django_graph_api/graphql/types.py
Outdated
self.arguments = arguments or {} | ||
self.description = description | ||
self.nullable = null |
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 think that it might be easier for folks to get used to the API if the attribute were also called null
checked boxes in the OP. Re: ManyRelatedField, I feel like the spec is kind of vague about this, but the intro to graphql is a bit more explicit. We should be able to handle nullable/non-null lists of nullable/non-null items. I'm not quite sure how that would play with ManyRelatedField... I don't know what would lead to an item in a ManyRelatedField coercing to null. FWIW I'm inclined to say that the |
Field(null=False)
to mark a field as non-nullableIssues:
ManyRelatedField(null=False)
?None
that is nullable and one that is not is that an error in theerrors
list is added to say that was not allowed. But if someone is not expecting that field to be null then tries to use the result without checking for errors, they will have problems. We also don't currently have a way of showing which error was for which field.Fixes #57