-
Notifications
You must be signed in to change notification settings - Fork 87
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
WIP - Allow inference of booleans in enums #1550
Conversation
Master PR for 0.8.1
Make master 0.9.0
Where a field is a BooleanField or a NullBooleanField inference should generate a lookup list with `[Yes, No]` and `[Yes, No, Unknown]` as options (for radio buttons). This will require changes to templates/radio.html so that Unkown does not use Unknown as a value, instead leaving the value empty so that we know the intended value is None. This would be easier if lookuplists were tuples (like choices in Django) so I'm considering implementing support for lists of tuples (as well as the current list of strings) for lookuplists - the alternative is too much logic in the template and special-casing the string 'Unknown' Will fix #1540
I think that's a good call. It means we'll need changes to do changes to search but maybe supporting choices like Django uses choices is the right thing to do. |
The other side of this argument is that lookup lists are exactly that, a list. So they won't have a different value, but I think that's ok. |
I was thinking of supporting both options actually, so you can have ["", ""] but for some cases (in particular this one) you can have [("",""), ("","")] |
self.assertIn('No', rendered) | ||
self.assertNotIn('Unknown', rendered) | ||
|
||
def test_radio_infer_lookuplists_with_quantum_boolean(self): |
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.
Best test name this week.
@@ -37,6 +37,7 @@ class HatWearer(models.EpisodeSubrecord): | |||
name = dmodels.CharField(max_length=200) | |||
hats = dmodels.ManyToManyField(Hat, related_name="hat_wearers") | |||
wearing_a_hat = dmodels.BooleanField(default=True) | |||
suits_a_hat = dmodels.NullBooleanField() |
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 we should pivot to this application.
ISWYM. It's quite a dirty hack to special case If we're implementing general 'accept tuples' then I reckon you'd want to:
And I think we have to update And presumably something in |
@davidmiller I think that's a great call, stick the logic in get_field_enum, return a dictionary. It will require special casing in extract.html and the two radio button templates. Yeah that's should work quite elegantly. |
For the sake of expedience, and not leaving this PR open for ever, I was investigating the non-elegant special-case the enum in radio.html and it happens that the mix of angular template and django template is pretty horrible if you need to do that sort of thing. Mostly because the django template runs first, and if the template is bound to an angular [[ ]] then that data isn't available to django :( Sorry, just needed to vent a bit :) |
Let it all out! yeah if you wanted to iterate over something a key values of a js object sometimes but an array other times that get tricky... I think there's an argument to changing lookup lists to dictionaries but that's way beyond the scope. Other option would be to change it so that it evaluates the label item (ie not the one on the ng-model) if item == True, item == False, item == None, then put in a label. I'm not sure that's that nice either... |
Two other options: You can look at the field type pre save and convert the string You can check the field type and the value "Unknown" combination in Either of these allow you to continue as if this is any other field with an enum/lookuplist as far as the template is concerned. A Django |
…t.demographics_set.x()
Fixup: Re-word text that was apparently copy and pasted from Wardrounds.
…'t break old versions of the docs
fixes some of the links in the tutorial and puts in a more up to date…
…TemplateView as no context is requried.
The relevant controller was removed in 0.8 and this URL/View/Template is not used by any known application.
Remove unused template views
…views 1341 logicless template views
Move Pathway and Search Javascripts into the karma_defauts.js
…-delete 1480 delete recordeditor delete
Re-pointed at 0.13 so closing this one... |
Where a field is a BooleanField or a NullBooleanField inference should
generate a lookup list with
[Yes, No]
and[Yes, No, Unknown]
asoptions (for radio buttons) respectively.
This will require changes to templates/radio.html so that Unknown does
not use Unknown as a value, instead leaving the value empty so that we
know the intended value is None.
This would be easier if lookuplists were tuples (like choices in Django)
so I'm considering implementing support for lists of tuples (as well as
the current list of strings) for lookuplists - the alternative is too
much logic in the template and special-casing the string 'Unknown'.
Does this make sense @fredkingham or should I just implement a helper
for converting Unknown -> "" in this specific case?
TODO
Will fix #1540