Cleanup of for sharing it more consistently across the code #1136

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

pablorecio commented May 18, 2013

Related to ticket #20450

@ambv ambv commented on the diff May 18, 2013

django/forms/models.py
@@ -1002,13 +1002,14 @@ class ModelChoiceField(ChoiceField):
' the available choices.'),
}
- def __init__(self, queryset, empty_label="---------", cache_choices=False,
+ def __init__(self, queryset, empty_label=False, cache_choices=False,
@ambv

ambv May 18, 2013

Contributor

Cleaner API with empty_label=None

@pablorecio

pablorecio May 18, 2013

Contributor

Yeah it's cleaner but it break the tests. None is the used value for saying "I don't want an empty_label". So if we use None there isn't any way of knowing what we want: use the default label, or we don't want any label

@pablorecio

pablorecio May 19, 2013

Contributor

And I didn't do def __init__(self, queryset, empty_label=VERBOSE_BLANK_CHOICE_DASH, cache_choices=False, cause I feel that is even uglier than using False.

@ambv

ambv May 19, 2013

Contributor

In this case the current state of things is preferrable to this pull request.

👎

@pablorecio

pablorecio May 19, 2013

Contributor

Why? I think that having the same string in different places isn't consistent enough. Having a common point is much more reliable for future changes.

@ambv

ambv May 19, 2013

Contributor
  • More code
  • Less obvious code than the original
  • Overly long constant name
  • Style not used in the rest of Django
@pablorecio

pablorecio May 19, 2013

Contributor

Ok, fair enough

@ambv ambv commented on the diff May 18, 2013

django/forms/models.py
required=True, widget=None, label=None, initial=None,
help_text='', to_field_name=None, *args, **kwargs):
if required and (initial is not None):
self.empty_label = None
else:
- self.empty_label = empty_label
+ from django.db.models.fields import VERBOSE_BLANK_CHOICE_DASH
+ self.empty_label = empty_label if empty_label is not False else VERBOSE_BLANK_CHOICE_DASH
@ambv

ambv May 18, 2013

Contributor

Better yet: empty_label or VERBOSE_BLANK_CHOICE_DASH

@pablorecio

pablorecio May 18, 2013

Contributor

It does came for the same reason that above's comment.

pablorecio closed this May 19, 2013

pablorecio deleted the pablorecio:cleanup-empty-label-choices branch May 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment