Proposal: upgrading the choices machinery for Django

[Łukasz Langa]

A nice HTML rendering of this proposal is available at:
http://lukasz.langa.pl/2/upgrading-choices-machinery-django/

==========================================
Upgrading the choices machinery for Django
==========================================

Specifying choices for form fields and models currently does not do much justice
to the `DRY
<https://docs.djangoproject.com/en/1.4/misc/design-philosophies/#don-t-repeat-yourself-dry>`_
philosophy Django is famous for. Everybody seems to either have their own way of
working around it or live with the suboptimal tuple-based pairs. This Django
enhancement proposal presents a comprehensive solution based on an existing
implementation, explaining reasons behind API decisions and their implications
on the framework in general.

Current status
--------------

The current way of specifying choices for a field is as follows::

GENDER_CHOICES = (
(0, 'male'),
(1, 'female'),
(2, 'not specified'),
)

class User(models.Model):
gender = models.IntegerField(choices=GENDER_CHOICES)

When I then want to implement behaviour which depends on the ``User.gender``
value, I have a couple of possibilities. I've seen all in production code which
makes it all the more scary.

Way 1
~~~~~

Use ``get_FIELD_display``::

GENDER_CHOICES = (
(0, 'male'),
(1, 'female'),
(2, 'not specified'),
)

class User(models.Model):
gender = models.IntegerField(choices=GENDER_CHOICES)

def greet(self):
gender = self.get_gender_display()
if gender == 'male':
return 'Hi, boy.'
elif gender == 'female':
return 'Hello, girl.'
else:
return 'Hey there, user!'

This will fail once you start translating the choice descriptions or if you
rename them and is generally brittle. So we have to improve on it by using the
numeric identifiers.

Way 2
~~~~~

Use numeric IDs::

GENDER_CHOICES = (
(0, _('male')),
(1, _('female')),
(2, _('not specified')),
)

class User(models.Model):
gender = models.IntegerField(choices=GENDER_CHOICES,
default=2)

def greet(self):
if self.gender == 0:
return 'Hi, boy.'
elif self.gender == 1:
return 'Hello, girl.'
else:
return 'Hey there, user!'

This is just as bad because once the identifiers change, it's not trivial to
grep for existing usage, let alone 3rd party usage. This looks less wrong when
using ``CharFields`` instead of ``IntegerFields`` but the problem stays the
same. So we have to improve on it by explicitly naming the options.

Way 3
~~~~~

Explicit choice values::

GENDER_MALE = 0
GENDER_FEMALE = 1
GENDER_NOT_SPECIFIED = 2

GENDER_CHOICES = (
(GENDER_MALE, _('male')),
(GENDER_FEMALE, _('female')),
(GENDER_NOT_SPECIFIED, _('not specified')),
)

class User(models.Model):
gender = models.IntegerField(choices=GENDER_CHOICES,
default=GENDER_NOT_SPECIFIED)

def greet(self):
if self.gender == GENDER_MALE:
return 'Hi, boy.'
elif self.gender == GENDER_NOT_SPECIFIED:
return 'Hello, girl.'
else: return 'Hey there, user!'

This is a saner way but starts getting overly verbose and redundant. You can
improve encapsulation by moving the choices into the ``User`` class but that on
the other hand beats reusability.

The real problem however is that there is no `One Obvious Way To Do It
<http://www.python.org/dev/peps/pep-0020/>`_ and newcomers are likely to choose
poorly.

tl;dr or The Gist of It
-----------------------

My proposal suggests embracing a solution that is already implemented and tested
with 100% statement coverage. It's easily installable::

pip install dj.choices

and lets you write the above example as::

from dj.choices import Choices, Choice

class Gender(Choices):
male = Choice("male")
female = Choice("female")
not_specified = Choice("not specified")

class User(models.Model):
gender = models.IntegerField(choices=Gender(),
default=Gender.not_specified.id)

def greet(self):
gender = Gender.from_id(self.gender)
if gender == Gender.male:
return 'Hi, boy.'
elif gender == Gender.female:
return 'Hello, girl.'
else:
return 'Hey there, user!'

or using the provided ``ChoiceField`` (fully compatible with
``IntegerFields``)::

from dj.choices import Choices, Choice
from dj.choices.fields import ChoiceField

class Gender(Choices):
male = Choice("male")
female = Choice("female")
not_specified = Choice("not specified")

class User(models.Model):
gender = ChoiceField(choices=Gender,
default=Gender.not_specified)

def greet(self):
if self.gender == Gender.male:
return 'Hi, boy.'
elif self.gender == Gender.female:
return 'Hello, girl.'
else:
return 'Hey there, user!'

BTW, the reason choices need names as arguments is so they can be picked up by
``makemessages`` and translated.

But it's much more than that so read on.

The Choices class
-----------------

By default the ``Gender`` class has its choices enumerated similarly to how
Django models order their fields. This can be seen by instantiating it::

>>> Gender()
[(1, u'male'), (2, u'female'), (3, u'not specified')]

By default an item contains the numeric ID and the localized description. The
format can be customized, for instance for ``CharField`` usage::

>>> Gender(item=lambda c: (c.name, c.desc))
[(u'male', u'male'), (u'female', u'female'), (u'not_specified', u'not specified')]

But that will probably make more sense with a foreign translation::

>>> from django.utils.translation import activate
>>> activate('pl')
>>> Gender(item=lambda c: (c.name, c.desc))
[(u'male', u'm\u0119\u017cczyzna'), (u'female', u'kobieta'), (u'not_specified', u'nie podano')]

It sometimes makes sense to provide only a subset of the defined choices::

>>> Gender(filter=('female', 'not_specified'), item=lambda c: (c.name, c.desc))
[(u'female', u'kobieta'), (u'not_specified', u'nie podano')]

A single Choice
---------------

Every ``Choice`` is an object::

>>> Gender.female
<Choice: female (id: 2, name: female)>

It contains a bunch of attributes, e.g. its name::

>>> Gender.female.name
u'female'

which probably makes more sense if accessed from a database model (in the
following example, using a ``ChoiceField``)::

>>> user.gender.name
u'female'

Other attributes include the numeric ID, the localized and the raw description
(the latter is the string as present before the translation)::

>>> Gender.female.id
2
>>> Gender.female.desc
u'kobieta'
>>> Gender.female.raw
'female'

Within a Python process, choices can be compared using identity comparison::

>>> u.gender
<Choice: male (id: 1, name: male)>
>>> u.gender is Gender.male
True

Across processes the serializable value (either ``id`` or ``name``) should be
used. Then a choice can be retrieved using a class-level getter::

>>> Gender.from_id(3)
<Choice: not specified (id: 3, name: not_specified)>
>>> Gender.from_name('male')
<Choice: male (id: 1, name: male)>

Grouping choices
----------------

One of the problems with specifying choice lists is their weak extensibility.
For instance, an application defines a group of possible choices like this::

>>> class License(Choices):
... gpl = Choice("GPL")
... bsd = Choice("BSD")
... proprietary = Choice("Proprietary")
...
>>> License()
[(1, u'GPL'), (2, u'BSD'), (3, u'Proprietary')]

All is well until the application goes live and after a while the developer
wants to include LGPL. The natural choice would be to add it after gpl but when
we do that, the indexing would break. On the other hand, adding the new entry at
the end of the definition looks ugly and makes the resulting combo boxes in the
UI sorted in a counter-intuitive way. Grouping lets us solve this problem by
explicitly defining the structure within a class of choices::

>>> class License(Choices):
... COPYLEFT = Choices.Group(0)
... gpl = Choice("GPL")
...
... PUBLIC_DOMAIN = Choices.Group(100)
... bsd = Choice("BSD")
...
... OSS = Choices.Group(200)
... apache2 = Choice("Apache 2")
...
... COMMERCIAL = Choices.Group(300)
... proprietary = Choice("Proprietary")
...
>>> License()
[(1, u'GPL'), (101, u'BSD'), (201, u'Apache 2'), (301, u'Proprietary')]

This enables the developer to include more licenses of each group later on::

>>> class License(Choices):
... COPYLEFT = Choices.Group(0)
... gpl_any = Choice("GPL, any")
... gpl2 = Choice("GPL 2")
... gpl3 = Choice("GPL 3")
... lgpl = Choice("LGPL")
... agpl = Choice("Affero GPL")
...
... PUBLIC_DOMAIN = Choices.Group(100)
... bsd = Choice("BSD")
... public_domain = Choice("Public domain")
...
... OSS = Choices.Group(200)
... apache2 = Choice("Apache 2")
... mozilla = Choice("MPL")
...
... COMMERCIAL = Choices.Group(300)
... proprietary = Choice("Proprietary")
...
>>> License()
[(1, u'GPL, any'), (2, u'GPL 2'), (3, u'GPL 3'), (4, u'LGPL'),
(5, u'Affero GPL'), (101, u'BSD'), (102, u'Public domain'),
(201, u'Apache 2'), (202, u'MPL'), (301, u'Proprietary')]

The behaviour in the example above was as follows:

- the developer renamed the GPL choice but its meaning and ID remained stable

- BSD, Apache and proprietary choices have their IDs unchanged

- the resulting class is self-descriptive, readable and extensible

The explicitly specified groups can be used as other means of filtering, etc.::

>>> License.COPYLEFT
<ChoiceGroup: COPYLEFT (id: 0)>
>>> License.gpl2 in License.COPYLEFT.choices
True
>>> [(c.id, c.desc) for c in License.COPYLEFT.choices]
[(1, u'GPL, any'), (2, u'GPL 2'), (3, u'GPL 3'), (4, u'LGPL'),
(5, u'Affero GPL')]

Pushing polymorphism to the limit - extra attributes
----------------------------------------------------

Let's see our original example once again::

from dj.choices import Choices, Choice
from dj.choices.fields import ChoiceField

class Gender(Choices):
male = Choice("male")
female = Choice("female")
not_specified = Choice("not specified")

class User(models.Model):
gender = ChoiceField(choices=Gender,
default=Gender.not_specified)

def greet(self):
if self.gender == Gender.male:
return 'Hi, boy.'
elif self.gender == Gender.female:
return 'Hello, girl.'
else:
return 'Hey there, user!'

If you treat DRY really seriously, you'll notice that actually separation
between the choices and the greetings supported by each of them may be
a violation of the "Every distinct concept and piece of data should live in one,
and only one, place" rule. You might want to move this information up::

from dj.choices import Choices, Choice
from dj.choices.fields import ChoiceField

class Gender(Choices):
male = Choice("male").extra(
hello='Hi, boy.')
female = Choice("female").extra(
hello='Hello, girl.')
not_specified = Choice("not specified").extra(
hello='Hey there, user!')

class User(models.Model):
gender = ChoiceField(choices=Gender,
default=Gender.not_specified)

def greet(self):
return self.gender.hello

As you see, the ``User.greet()`` method is now virtually gone. Moreover, it
already supports whatever new choice you will want to introduce in the future.
This way the choices class starts to be the canonical place for the concept it
describes. Getting rid of chains of ``if`` - ``elif`` statements is now just
a nice side effect.

.. note::

I'm aware this is advanced functionality. This is not a solution for every
case but when it is needed, it's priceless.

Advantages of merging this solution
-----------------------------------

1. Having a single source of information, whether it is a list of languages,
genders or states in the USA, is DRY incarnate.

2. If necessary, this source can later be filter and reformatted upon
instatiation.

3. Using ``ChoiceFields`` or explicit ``from_id()`` class methods on choices
enables cleaner user code with less redundancy and dependency on hard-coded
values.

4. Upgrading the framework's choices to use class-based choices will increase
its DRYness and enable better future extensibility.

5. Bikeshedding on the subject will hopefully stop.

Disadvantages
-------------

1. A new concept in the framework increases the vocabulary necessary to
understand what's going on.

2. Because of backwards compatibility in order to provide a *One Obvious Way To
Do It* we actually add another way to do it. We can however endorse it as the
recommended way.

Performance considerations
--------------------------

Creation of the proper ``Choices`` subclass uses a metaclass to figure out
choice order, group membership, etc. This is done once when the module
containing the class is loaded.

After instantiation the resulting object is little more than a raw list of
pairs.

Using ``ChoiceFields`` instead of raw ``IntegerFields`` introduces automatic
choice unboxing and moves the field ID checks up the stack introducing
negligible performance costs. I don't personally believe in microbenchmarks so
didn't try any but can do so if requested.

FAQ
---

Is it used anywhere already?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Yes, it is. It grew out of a couple of projects I did over the years, for
instance `allplay.pl <http://allplay.pl/>`_ or `spiralear.com
<http://spiralear.com/en/>`_. Various versions of the library are also used by
my former and current employers. It has 100% statement coverage in unit tests.

Has anyone evaluated it yet?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I have shown this implementation to various people during PyCon US 2012 and it
gathered some enthusiasm. Jannis Leidel, Carl Meyer and Julien Phalip seemed
interested in the idea at the very least.

Why not use an existing Django enum implementation?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The most popular are:

- `django-choices <http://pypi.python.org/pypi/django-choices>`_ by Jason Webb.
The home page link is dead, the implementation lacks grouping support,
internationalization support and automatic integer ID incrementation. Also,
I find the reversed syntax a bit unnatural. There are some tests.

- `django-enum <pypi.python.org/pypi/django-enum>`_ by Jacob Smullyan. There is
no documentation nor tests, the API is based on a syntax similar to
namedtuples with a single string of space-separated choices. Doesn't support
groups nor internationalization.

Why not use a generic enum implementation?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The most popular are:

- `enum <http://pypi.python.org/pypi/enum>`_ by Ben Finney (also a rejected `PEP
<http://www.python.org/dev/peps/pep-0354/>`_) - uses the "object
instantiation" approach which I find less readable and extensible.

- `flufl.enum <https://launchpad.net/flufl.enum>`_ by Barry Warsaw - uses the
familiar "class definition" approach but doesn't support internationalization
and grouping.

Naturally, none of the generic implementations provide shortcuts for Django
forms and models.

Why not use namedtuples?
~~~~~~~~~~~~~~~~~~~~~~~~

This doesn't solve anything because a ``namedtuple`` defines a type that is
later populated with data on a per-instance basis. Defining a type first and
then instantiating it is already clumsy and redundant.

Do you have a field like ``ChoiceField`` but holding characters?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Not yet but this is planned.

I don't like having to write ``Choice()`` all the time.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

You can use a trick::

class Gender(Choices):
_ = Choices.Choice

male = _("male")
female = _("female")
not_specified = _("not specified")

Current documentation for the project uses that because outside of Django core
this is necessary for ``makemessages`` to pick up the string for translation.
Once merged this will not be necessary so **this is not a part of the
proposal**. You're free to use whatever you wish, like importing ``Choice`` as
``C``, etc.

--
Best regards,
Łukasz Langa
Senior Systems Architecture Engineer

IT Infrastructure Department
Grupa Allegro Sp. z o.o.

http://lukasz.langa.pl/
+48 791 080 144

[Dan Poirier]

I like this - it solves something that's always bugged me.

One nice addition would be to specify explicitly what .id a particular
Choice should have. It would be useful in converting existing code that
might not have chosen to number its choices the same way this proposal
does by default. It looks like you could use Choices.Group(n-1) to make
the next choice defined use id=n, but that's not very elegant.

[Łukasz Langa]

Wiadomość napisana przez Dan Poirier w dniu 4 kwi 2012, o godz. 14:08:

> One nice addition would be to specify explicitly what .id a particular
> Choice should have. It would be useful in converting existing code that
> might not have chosen to number its choices the same way this proposal
> does by default. It looks like you could use Choices.Group(n-1) to make
> the next choice defined use id=n, but that's not very elegant.

Right. The ability to explicitly set the .id on a single choice is useful.

The only reason it's not already there is that I didn't want to write my own
`makemessages`. Then again I might do just that. My proposal is based on
a working implementation because I didn't want to fall into bikeshedding on
what could have been theoretically implemented.

What we need now to push things forward is a list of +1, +1 if ..., +0,
+0 if ..., -1, "how dare you?!" from a bunch of people.

[Thomas Guettler]

Am 03.04.2012 21:31, schrieb Łukasz Langa:
> A nice HTML rendering of this proposal is available at:
> http://lukasz.langa.pl/2/upgrading-choices-machinery-django/
>
>
>
> ==========================================
> Upgrading the choices machinery for Django
> ==========================================

> ...

since most people have something like this in their code, a better solution should require
only some additional characters.

GENDER_CHOICES = (
(0, 'male'),
(1, 'female'),
(2, 'not specified'),
)

# if this would be supported, updating the code would be easy
GENDER_CHOICES = Choices(

(0, 'male'),
(1, 'female'),
(2, 'not specified'),
)

#about Group

>>> class License(Choices):
... COPYLEFT = Choices.Group(0)
... gpl_any = Choice("GPL, any")

I don't like Choices.Group. I think it is better if choices must be set explicit, not
implicit (auto-increment).

Here is my solution, which is similar to way3 in your proposal.

{{{

class ChoiceDict(SortedDict):
'''
Iterating this SortedDict will return
the items (and not the keys). This is handy if
you want to uses this for a choice list on
a model field
'''
__iter__=SortedDict.iteritems

class Foo(models.Model):
STATE_NEW='new'
STATE_DONE='done'
states=ChoiceDict((STATE_NEW, _('new')),
(STATE_DONE, _('done')),
)
state=models.CharField(max_length=16, verbose_name=u'Status', default=STATE_NEU, choices=states)
}}}
I like this since, you can access the verbose string like this: Foo.states[obj.state]

--
Thomas Guettler, http://www.thomas-guettler.de/
E-Mail: guettli (*) thomas-guettler + de

[Łukasz Langa]

Wiadomość napisana przez Thomas Guettler w dniu 4 kwi 2012, o godz. 15:55:

> since most people have something like this in their code, a better solution should require
> only some additional characters.

Thank you for your feedback. I disagree since it wouldn't address any of the original concerns from the proposal. Most importantly, reducing redundancy and creating a canonical place for storing information of a specific concept.

> I think it is better if choices must be set explicit, not
> implicit (auto-increment).

As Dan Poirier pointed out, setting an explicit .id on a single Choice will let you do that.

> Here is my solution, which is similar to way3 in your proposal.

Doesn't support groups, uses the tuple-galore syntax, still leaves you with redundancy of having to first define available options and later building a choice dictionary.

[Daniel Sokolowski]

Hi Lukash, I'm 0 to this.

I find it well thought out but in my use cases I fail to see the advantage
over the WAY 3 (or WAY 4 pointed out below) and it adds a layer of
abstraction where a straight tuple or a list does the job well enough.

The choice filtering could come in handy and in the end I don't see why this
can't co-exist with django's standard way.

WAY 4:

class UserManager(models.Manager):

GENDER_MALE = 0
GENDER_FEMALE = 1
GENDER_NOT_SPECIFIED = 2
GENDER_CHOICES = (
(GENDER_MALE, _('male')),
(GENDER_FEMALE, _('female')),
(GENDER_NOT_SPECIFIED, _('not specified')),
)

def males(self):
""" Returns all entries accessible through front end site"""
return self.all().filter(gender=self.GENDER_MALE)
def females(self):
""" Returns all entries accessible through front end site"""
return self.all().filter(gender=self.GENDER_FEMALE)
...

class User(models.Model):
gender = models.IntegerField(choices=UserManager.GENDER_CHOICES,
default=UserManager.GENDER_NOT_SPECIFIED)
objects = UserManager()

ef greet(self):
if self.gender == UserManager.GENDER_MALE:
return 'Hi, boy.'
elif self.gender == UserManager.GENDER_NOT_SPECIFIED:

return 'Hello, girl.'
else: return 'Hey there, user!'

-----Original Message-----
From: �ukasz Langa
Sent: Wednesday, April 04, 2012 9:15 AM
To: django-d...@googlegroups.com
Subject: Re: Proposal: upgrading the choices machinery for Django

Wiadomo�� napisana przez Dan Poirier w dniu 4 kwi 2012, o godz. 14:08:

> One nice addition would be to specify explicitly what .id a particular
> Choice should have. It would be useful in converting existing code that
> might not have chosen to number its choices the same way this proposal
> does by default. It looks like you could use Choices.Group(n-1) to make
> the next choice defined use id=n, but that's not very elegant.

Right. The ability to explicitly set the .id on a single choice is useful.

The only reason it's not already there is that I didn't want to write my own
`makemessages`. Then again I might do just that. My proposal is based on
a working implementation because I didn't want to fall into bikeshedding on
what could have been theoretically implemented.

What we need now to push things forward is a list of +1, +1 if ..., +0,
+0 if ..., -1, "how dare you?!" from a bunch of people.

--
Best regards,
�ukasz Langa
Senior Systems Architecture Engineer

IT Infrastructure Department
Grupa Allegro Sp. z o.o.

http://lukasz.langa.pl/
+48 791 080 144

--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to
django-develop...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.

[Tom Evans]

2012/4/3 Łukasz Langa <luk...@langa.pl>:

> A nice HTML rendering of this proposal is available at:
> http://lukasz.langa.pl/2/upgrading-choices-machinery-django/

Disclaimer: all my code currently looks like 'Way 3'.

In general, I like it. There are a few things I don't like in it:

The class definition grates. When we say things like:

class Gender(Choices):
male = Choice("male")

That says to me that Gender.male is mutable. Ick.

There is no easy way by inspecting the code to see what choice a value
of 7 equates to any-more. Do the choices start from 0 or do they start
from 1, as inferred by the examples (Why? What is wrong with 0?).
Repetition isn't good, but ambiguity is worse.

I don't like the grouping at all. Grouping under the old system was
clean and simple, it didn't require magic arguments, groups didn't
require that all options are contiguous in a single range.

This grouping system just seems destined for data loss/confusion. If I
want to split a group in two, the enums in the new group change
values! That is not a good approach, and was not necessary with the
old system.

This grouping system cannot logically arrange the options into groups,
since it relies on them being attributes of the class, and so you need
to invent a way of grouping. If I had a vote, I'd be strongly -1 on
any proposal with this sort of grouping, it seems dangerous and wrong.

Finally, the proposal seems to concentrate solely on choices as an
enum. The proposal completely ignores that choices can be almost any
kind of value, eg:

MALE="m"
FEMALE="f"
UNKNOWN="?"
GENDER_CHOICES = (
(MALE, "Male"),
(FEMALE, "Female"),
(UNKNOWN, "Unknown"),
)

this would be an entirely appropriate choices for a CharField.

Cheers

Tom

[Adrian Holovaty]

2012/4/3 Łukasz Langa <luk...@langa.pl>:

> Explicit choice values::
>
>  GENDER_MALE = 0
>  GENDER_FEMALE = 1
>  GENDER_NOT_SPECIFIED = 2
>
>  GENDER_CHOICES = (
>      (GENDER_MALE, _('male')),
>      (GENDER_FEMALE, _('female')),
>      (GENDER_NOT_SPECIFIED, _('not specified')),
>  )
>
>  class User(models.Model):
>      gender = models.IntegerField(choices=GENDER_CHOICES,
>              default=GENDER_NOT_SPECIFIED)
>
>      def greet(self):
>          if self.gender == GENDER_MALE:
>              return 'Hi, boy.'
>          elif self.gender == GENDER_NOT_SPECIFIED:
>              return 'Hello, girl.'
>          else: return 'Hey there, user!'
>
> This is a saner way but starts getting overly verbose and redundant. You can
> improve encapsulation by moving the choices into the ``User`` class but that on
> the other hand beats reusability.

I don't see the immediate need for Yet Another Sub-framework, as
described in this proposal. This is what I normally do, and it works
fine:

class User(models.Model):
MALE = 0
FEMALE = 1
GENDERS = [(MALE, 'Male'), (FEMALE, 'Female')]
gender = models.IntegerField(choices=GENDERS)

def greet(self):
return {MALE: 'Hi, boy', FEMALE: 'Hi, girl.'}[self.gender]

If people aren't understanding that, we should improve our documentation.

Also, the fact that we *don't* have a sub-framework for this lets
people do whatever they want -- including using the dj.choices module.
So I am -1 on including this in Django proper.

Adrian

[Sean Brant]

I agree we don't really gain anything from including this in core.
Django model utils[1] has a pretty solid implementation of a choice
abstraction.

[1] https://github.com/carljm/django-model-utils

[Łukasz Langa]

Wiadomość napisana przez Daniel Sokolowski w dniu 4 kwi 2012, o godz. 17:52:

The choice filtering could come in handy and in the end I don't see why this can't co-exist with django's standard way.

WAY 4:

Nice but very verbose.

-- 
Best regards,
Łukasz Langa

Senior Systems Architecture Engineer

IT Infrastructure Department
Grupa Allegro Sp. z o.o.

[Łukasz Langa]

Wiadomość napisana przez Tom Evans w dniu 4 kwi 2012, o godz. 18:40:

> The class definition grates. When we say things like:
>
> class Gender(Choices):
> male = Choice("male")
>
> That says to me that Gender.male is mutable. Ick.

Thanks for your feedback. Do model and form field definitions say that as well? It's true that I could have block users from mutating the value but "we're all consenting adults here", right?

> There is no easy way by inspecting the code to see what choice a value
> of 7 equates to any-more.

True, the same goes for spotting errors with manual numbering when there are many values.

> Do the choices start from 0 or do they start
> from 1, as inferred by the examples (Why? What is wrong with 0?).
> Repetition isn't good, but ambiguity is worse.

It's a matter of aesthetics and as such it's totally subjective. I made the values start from 1 so that the first Choice.Group can have value 0 and not -1 which looks ugly.

> This grouping system just seems destined for data loss/confusion. If I
> want to split a group in two, the enums in the new group change
> values! That is not a good approach, and was not necessary with the
> old system.

I can't see how they have to.

> If I had a vote, I'd be strongly -1 on
> any proposal with this sort of grouping, it seems dangerous and wrong.

Can you elaborate on what is dangerous about them?

> Finally, the proposal seems to concentrate solely on choices as an
> enum. The proposal completely ignores that choices can be almost any
> kind of value, eg:
>
> MALE="m"
> FEMALE="f"
> UNKNOWN="?"
> GENDER_CHOICES = (
> (MALE, "Male"),
> (FEMALE, "Female"),
> (UNKNOWN, "Unknown"),
> )
>
> this would be an entirely appropriate choices for a CharField.

Using code in my proposal:

>>> class Gender(Choices):
... m = Choice("male")
... f = Choice("female")
... n = Choice("not specified")
...
>>> Gender(item=lambda c: (c.name, c.desc))
[(u'm', u'male'), (u'f', u'female'), (u'n', u'not specified')]

[Łukasz Langa]

Wiadomość napisana przez Adrian Holovaty w dniu 4 kwi 2012, o godz. 18:41:

> Also, the fact that we *don't* have a sub-framework for this lets
> people do whatever they want -- including using the dj.choices module.
> So I am -1 on including this in Django proper.

Thank you for your feedback. As you're one of the BFDLs for the project, that basically closes the proposal as rejected, am I right?

[Daniel Greenfeld]

<delurk>

+1 to what Adrian says. In fact...

On two occasions now I've had to do serious debugging on implementations done by other people who used a technique not dissimilar to what Łukasz proposes. In both cases, while the inheritance and better control structures were nice, the issue was that if you give people enough rope to hang themselves, they will.

The current system works very well, especially when you follow Adrian's pattern. Lukasz' system works well too. Therefore...

I submit that we keep Django core for choices as is and let people who want to use another system implement 3rd party packages. 

</delurk> 

[Łukasz Langa]

Wiadomość napisana przez Daniel Greenfeld w dniu 4 kwi 2012, o godz. 22:48:

> +1 to what Adrian says.

Thanks for your input! :)

> On two occasions now I've had to do serious debugging on implementations done by other people who used a technique not dissimilar to what Łukasz proposes. In both cases, while the inheritance and better control structures were nice, the issue was that if you give people enough rope to hang themselves, they will.

Can you elaborate on that a bit? Even if the proposal is rejected in the end, I might use your experience to at least make the rope less deadly.

[Alex Ogier]

On Wed, Apr 4, 2012 at 5:18 PM, Łukasz Langa <luk...@langa.pl> wrote:

Wiadomość napisana przez Daniel Greenfeld w dniu 4 kwi 2012, o godz. 22:48:

> On two occasions now I've had to do serious debugging on implementations done by other people who used a technique not dissimilar to what Łukasz proposes. In both cases, while the inheritance and better control structures were nice, the issue was that if you give people enough rope to hang themselves, they will.

Can you elaborate on that a bit? Even if the proposal is rejected in the end, I might use your experience to at least make the rope less deadly.

I imagine a big one is that without an explicit database representation for every item, it is easy to get out of sync with the real data you have. And when that happens, there isn't necessarily a good way of retrieving the meaning of old data: if some data in your database says that you are using license '302' but that index was implicitly auto-generated by the particular ordering of Choice() instantiations, then when the ordering changes you can lose track (and indexing based on an implicit ordering of class attributes definitely seems shaky to me).

When I use enumerated fields, I generally make them VARCHAR(10) or so, and then use plain English, potentially abbreviated, for a database representation. That makes for reasonable code, "if self.license == 'gpl_any':" looks very readable to me, and doesn't restrict you from altering display values for internationalization.

It seems to me like this is really a documentation problem, where distilling the wisdom of developers like Adrian into a little best practices paragraph in the choices argument reference would go very far in making the awkwardness go away.

Best,

Alex Ogier

[Stephen Burrows]

I generally like this idea, except for the implicit integer ids based
on declaration order. As people have said, it seems like just asking
for data corruption. I'd much rather see implicit character ids based
on the attribute name of the choice, analogous to django fields.

Will you be making this project available as a third-party app?

--Stephen

[Thomas Guettler]

Hi,

I created a ticket, incl. patch

https://code.djangoproject.com/ticket/18062

--

[Łukasz Rekucki]

On 5 April 2012 09:45, Thomas Guettler <h...@tbz-pariv.de> wrote:
> Hi,
>
> I created a ticket, incl. patch
>
> https://code.djangoproject.com/ticket/18062
>
>

While the example itself is useful, I don't really think Django's
documentation should state obvious facts about Python, especially
false ones.

>
> Am 04.04.2012 18:41, schrieb Adrian Holovaty:
>>
>>
>> I don't see the immediate need for Yet Another Sub-framework, as
>> described in this proposal. This is what I normally do, and it works
>> fine:
>>
>> class User(models.Model):
>>     MALE = 0
>>     FEMALE = 1
>>     GENDERS = [(MALE, 'Male'), (FEMALE, 'Female')]
>>     gender = models.IntegerField(choices=GENDERS)
>>
>>     def greet(self):
>>         return {MALE: 'Hi, boy', FEMALE: 'Hi, girl.'}[self.gender]
>>

I' sure you meant:

def greet(self):
return {self.MALE: 'Hi, boy', self.FEMALE: 'Hi, girl.'}[self.gender]

Unless you defined MALE/FEMALE as globals too :) Otherwise you'll get
a NameError.

--
Łukasz Rekucki

[Łukasz Langa]

Wiadomość napisana przez Łukasz Rekucki w dniu 5 kwi 2012, o godz. 10:03:

> I' sure you meant:
>
> def greet(self):
> return {self.MALE: 'Hi, boy', self.FEMALE: 'Hi, girl.'}[self.gender]
>
> Unless you defined MALE/FEMALE as globals too :) Otherwise you'll get
> a NameError.

Also, a minor diversity note: in real life gender choice is not binary.

[Łukasz Langa]

Wiadomość napisana przez Stephen Burrows w dniu 5 kwi 2012, o godz. 07:41:

> I generally like this idea, except for the implicit integer ids based
> on declaration order.

Thanks for your input. I will add the ability to explicitly set .id values for each Choice in the next release.

> As people have said, it seems like just asking
> for data corruption.

Fair enough, although that does not match my experience.

> I'd much rather see implicit character ids based
> on the attribute name of the choice, analogous to django fields.

>>> class Gender(Choices):
... m = Choice("male")
... f = Choice("female")

... n = Choice("not specified")
...
>>> Gender(item=lambda c: (c.name, c.desc))
[(u'm', u'male'), (u'f', u'female'), (u'n', u'not specified')]

> Will you be making this project available as a third-party app?

pip install dj.choices

[Thomas Guettler]

Am 05.04.2012 10:03, schrieb Łukasz Rekucki:
> On 5 April 2012 09:45, Thomas Guettler<h...@tbz-pariv.de> wrote:
>> Hi,
>>
>> I created a ticket, incl. patch
>>
>> https://code.djangoproject.com/ticket/18062
>>
>>
>
> While the example itself is useful, I don't really think Django's
> documentation should state obvious facts about Python, especially
> false ones.

...

> Unless you defined MALE/FEMALE as globals too :) Otherwise you'll get
> a NameError.

patch is fixed now. Unfortunately I can't modify the description in trac.

Thomas

[Tom Evans]

2012/4/4 Łukasz Langa <luk...@langa.pl>:

> Wiadomość napisana przez Tom Evans w dniu 4 kwi 2012, o godz. 18:40:
>
>> The class definition grates. When we say things like:
>>
>>  class Gender(Choices):
>>    male = Choice("male")
>>
>> That says to me that Gender.male is mutable. Ick.
>
> Thanks for your feedback. Do model and form field definitions say that as well? It's true that I could have block users from mutating the value but "we're all consenting adults here", right?

Yes, they do. If I have a model with a field named 'address', I expect
that different instances of that model may have different values for
that attribute.

If on the other hand, I have a class with an attribute 'MALE', then I
expect that value to be immutable.

>
>> There is no easy way by inspecting the code to see what choice a value
>> of 7 equates to any-more.
>
> True, the same goes for spotting errors with manual numbering when there are many values.

That makes no sense. If you want to see what value a choice has with
the proposed system, you have to work out precisely what ids will be
automagically assigned to a particular choice.

If you want to spot errors with the current system, you simply look
for choices with the same number. You don't have to work out what
number a choice is, it is explicitly there.

>
>> Do the choices start from 0 or do they start
>> from 1, as inferred by the examples (Why? What is wrong with 0?).
>> Repetition isn't good, but ambiguity is worse.
>
> It's a matter of aesthetics and as such it's totally subjective. I made the values start from 1 so that the first Choice.Group can have value 0 and not -1 which looks ugly.
>
>> This grouping system just seems destined for data loss/confusion. If I
>> want to split a group in two, the enums in the new group change
>> values! That is not a good approach, and was not necessary with the
>> old system.
>
> I can't see how they have to.

So, If I take your examples for groups, and annotate it with the
automagic assigned id:

> class License(Choices):
> COPYLEFT = Choices.Group(0)
> gpl_any = Choice("GPL, any") # id: 1
> gpl2 = Choice("GPL 2") # id: 2
> gpl3 = Choice("GPL 3") # id: 2
> lgpl = Choice("LGPL") # id: 3
> agpl = Choice("Affero GPL") # id: 4
>
> PUBLIC_DOMAIN = Choices.Group(100)
> bsd = Choice("BSD") # id: 101
> public_domain = Choice("Public domain") # id: 102
>
> OSS = Choices.Group(200)
> apache2 = Choice("Apache 2") # id: 201
> mozilla = Choice("MPL") # id: 202

and now I decide that I want BSD licenses as a separate group to
public domain licenses, as they are not the same thing.

> class License(Choices):
> COPYLEFT = Choices.Group(0)
> gpl_any = Choice("GPL, any") # id: 1
> gpl2 = Choice("GPL 2") # id: 2
> gpl3 = Choice("GPL 3") # id: 2
> lgpl = Choice("LGPL") # id: 3
> agpl = Choice("Affero GPL") # id: 4
>
> PUBLIC_DOMAIN = Choices.Group(100)
> public_domain = Choice("Public domain") # id: 101
>
> BSD = Choices.Group(150)
> bsd = Choice("BSD") # id: 151
>
> OSS = Choices.Group(200)
> apache2 = Choice("Apache 2") # id: 201
> mozilla = Choice("MPL") # id: 202

public_domain has changed from 102 => 101
bsd has changed from 101 => 151

These ids only change because ids in this system are handed out
automagically in order of appearance in the class, and grouping is
handled by order of appearance in the class.

>> If I had a vote, I'd be strongly -1 on
>> any proposal with this sort of grouping, it seems dangerous and wrong.
>
> Can you elaborate on what is dangerous about them?

So as above. Your counter will be "well, in those cases you need to
explicitly set an id on those choices". This relies on the person
making the changes realizing that there is magic happening, and take
that into account. That is dangerous compared to the current system,
where I would trust even a novice developer to change and move around
options.

This is because the current system is explicitly clear in what is happening.

>
>> Finally, the proposal seems to concentrate solely on choices as an
>> enum. The proposal completely ignores that choices can be almost any
>> kind of value, eg:
>>
>> MALE="m"
>> FEMALE="f"
>> UNKNOWN="?"
>> GENDER_CHOICES = (
>>    (MALE, "Male"),
>>    (FEMALE, "Female"),
>>    (UNKNOWN, "Unknown"),
>>  )
>>
>> this would be an entirely appropriate choices for a CharField.
>
> Using code in my proposal:
>
>>>> class Gender(Choices):
> ...     m = Choice("male")
> ...     f = Choice("female")
> ...     n = Choice("not specified")
> ...
>>>> Gender(item=lambda c: (c.name, c.desc))
> [(u'm', u'male'), (u'f', u'female'), (u'n', u'not specified')]
>

So instead of MALE and FEMALE, Gender.m? No thanks.

Also, your counter-example is not exactly equivalent, and shows one
other thing that I missed - now the only valid choices are those that
can be used as python identifiers. You could not continue to have '?'
as the chosen choice for UNKNOWN.

Cheers

Tom

[Alex Ogier]

> >> class User(models.Model):
> >>     MALE = 0
> >>     FEMALE = 1
> >>     GENDERS = [(MALE, 'Male'), (FEMALE, 'Female')]
> >>     gender = models.IntegerField(choices=GENDERS)
> >>
> >>     def greet(self):
> >>         return {MALE: 'Hi, boy', FEMALE: 'Hi, girl.'}[self.gender]
> >>
>
> I' sure you meant:
>
> def greet(self):
>    return {self.MALE: 'Hi, boy', self.FEMALE: 'Hi, girl.'}[self.gender]
>
> Unless you defined MALE/FEMALE as globals too :) Otherwise you'll get
> a NameError.
>
> --
> Łukasz Rekucki

As attributes of the class object I'm pretty sure they are in scope. No NameErrors there.

Best,
Alex Ogier

[Łukasz Rekucki]

That's not how it works. Code that executes when creating a new class
does not define a lexical scope. There is no such thing as "class
scope". Try it yourself:

http://ideone.com/xbr0q

--
Łukasz Rekucki

[Alex Ogier]

Interesting, you are right. It's still a little strange to me to be accessing class attributes via the self object, but I suppose it is more flexible than accessing them as User.MALE etc. which would make renaming the class awkward.

Best,
Alex Ogier