Skip to content
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

Fixed #23681, Fixed #27445 -- Doc'd setting choices for NullBooleanField widgets. #13437

Merged
merged 1 commit into from Oct 8, 2020

Conversation

jacobtylerwalls
Copy link
Member

@matthiask
Copy link
Contributor

Maybe I'm being dense but I still wouldn't know which values to provide to the choices argument. An example would be enormously helpful here, e.g. choices = [("Yes", _("Yes")), ("No", _("No")), ...] – or is it choices = [(0, ...), (1, ...), ...], or choices = [(True, ...), (False, ...), ...]? I'm always unsure.

@jacobtylerwalls
Copy link
Member Author

Agreed an example would be beneficial. In that case, how about moving this tip box to the documentation for models.BooleanField?

@matthiask
Copy link
Contributor

That may be a good idea anyway especially since NullBooleanField has already been deprecated (see https://docs.djangoproject.com/en/dev/releases/3.1/#deprecated-features-3-1)

@matthiask
Copy link
Contributor

Thanks for the example, that's much better!

Sorry for misleading you regarding the place where to put the tip though, I was confused and thought the tip was already part of the models documentation. Also, only models.NullBooleanField has been deprecated, forms.NullBooleanField is still fine. I think the tip is more useful as a part of the forms.NullBooleanField documentation... sorry for the back and forth.

@jacobtylerwalls
Copy link
Member Author

No worries, I sincerely welcome feedback on where this will be most useful. Will swap it in a sec.

@jacobtylerwalls
Copy link
Member Author

Just to clarify ... you suggest I put the tip on the forms.NullBooleanField docs so that a user realizes, hey I don't want forms.NullBooleanField at all when I can set choices upstream on models.BooleanField and get a forms.TypedChoiceField instead, which allows different widgets!

@matthiask
Copy link
Contributor

(I am confused and also confusing tonight, sorry.)

The ticket description mentions forms, that's why I thought that the forms documentation may be the best place to put this tip.

Now that the tip is part of the forms.NullBooleanField documentation I suggest you add a None choice and refer to NullBooleanField directly. The TypedChoiceField doesn't work in this case because it doesn't know how to coerce e.g. a string value of "False".

@jacobtylerwalls
Copy link
Member Author

Figuring out the contemporary relevance of an old ticket is tough -- but what's evident is that something needs to be documented.

My interpretation of the ticket was that OP wanted a way to set choices on a forms.NullBooleanField. The consensus was to document Select widgets as the way to do that. Then years go by and models.NullBooleanField is deprecated but models.BooleanField(null=True) appears. Now, if I take your suggested tip, I don't believe it works -- there appear to be some issues with some of the Select widgets interacting with forms.NullBooleanField that @dcsmith1 was looking at in #12946. So I was trying to step around this whole thing, per the last comment on ticket-23681 and Mariusz's comment on #12946 that really you can just set this all the way upstream on the model layer if you're happy with getting a TypedChoiceField. My first question is -- what is the coercion issue you are experiencing with the approach I suggest? I'm not seeing that, I just see booleans being evaluted.

In sum, this could either wait until #12946 is reviewed, or, it could proceed independently as a way to show folks how to get out of this jam entirely by accepting TypedChoiceField. Given that this ticket is a 6yo documentation ticket and we have another ticket for the development issue, that was my thought -- provide a sufficient way for a Django user to discover how to customize the choices on a models.BooleanField where null may be either True or False. But I'm open to taking another direction here.

@jacobtylerwalls jacobtylerwalls force-pushed the issue-23681 branch 2 times, most recently from a34b5c4 to fda80f4 Compare October 4, 2020 16:11
@jacobtylerwalls jacobtylerwalls changed the title Fixed #23681 -- Added tip for setting choices on BooleanField. Fixed #23681 -- Added tip for setting choices on Select widgets for NullBooleanField. Oct 4, 2020
@carltongibson carltongibson self-assigned this Oct 8, 2020
@carltongibson carltongibson changed the title Fixed #23681 -- Added tip for setting choices on Select widgets for NullBooleanField. Fixed #23681, Fixed #27445 -- Doc'd setting choices for NullBooleanField widgets. Oct 8, 2020
…eld widgets.

Thanks to David Smith for investigation and review.

Co-authored-by: Carlton Gibson <carlton.gibson@noumenal.es>
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

OK, super. Thanks all for the work here. 🥇

@felixxm felixxm merged commit d976c25 into django:master Oct 8, 2020
@jacobtylerwalls jacobtylerwalls deleted the issue-23681 branch October 8, 2020 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants