Skip to content

Add default value to generated enum if not included in choices #598

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

Closed
wants to merge 2 commits into from

Conversation

mvanlonden
Copy link
Member

@mvanlonden mvanlonden commented Mar 21, 2019

Closes #474

Django fields with default="something" may not be included in choices but the value is a valid initial value for the field. This adds an DEFAULT option to the enum generated if the default value is not present in the choices.

https://simpleisbetterthancomplex.com/tips/2016/07/25/django-tip-8-blank-or-null.html

@coveralls
Copy link

coveralls commented Mar 21, 2019

Coverage Status

Coverage increased (+0.02%) to 94.381% when pulling 4732b34 on mvanlonden:blank-fields into ea2cd98 on graphql-python:master.

@zbyte64
Copy link
Collaborator

zbyte64 commented Mar 22, 2019

I think a person should be able to define the label for the blank option. For django forms the pattern is that you explicitly define a blank choice or pass empty_label: https://stackoverflow.com/questions/14541074/empty-label-choicefield-django

Another concern is how this gets squared with the proposal of letting people explicitly define enums.

@mvanlonden
Copy link
Member Author

Ah didn't realize defining an empty option is more idiomatic. That makes sense

@mvanlonden mvanlonden reopened this Mar 26, 2019
@mvanlonden
Copy link
Member Author

@zbyte64 @firaskafri I've taken an alternative approach by inspecting the default value for a field. Often times a CharField will define "" as the default value and it is not included in the choices. This adds a DEFAULT value to the generated enum.

@mvanlonden mvanlonden changed the title Support blank fields with choices Add default value to generated enum if not included in choices Mar 26, 2019
@mvanlonden mvanlonden requested a review from danpalmer March 26, 2019 22:09
Copy link
Collaborator

@danpalmer danpalmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken (please say if I am!), I think choices is validate in the default field clean method, so unless the default value is in the choices, it's not a valid input. When we provide field choices as an enum, we're doing that to communicate the valid options in the type system, and allowing clients to depend on type safety to some extent.

I think that the choice enum should match what is valid for the field. I'd suggest that this problem could be solved by either providing the default value in a side-channel of some sort, perhaps elsewhere in the API, or alternatively, the default value could be added to the field choices. I don't think either of these solutions would be the responsibility of Graphene though.

What do you think? I'm open to other solutions!

@mvanlonden
Copy link
Member Author

Yes @danpalmer after further investigation it looks like you're right. Previously wasn't able to find examples where an empty value was included in choices. https://github.com/django/django/blob/1123f4551158b7fc65d3bd88c375a4517dcd0720/tests/forms_tests/models.py#L37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants