Opened 12 years ago

Closed 12 years ago

Last modified 11 years ago

#18062 closed Cleanup/optimization (fixed)

Document Best Practice for Choices in Model Fields

Reported by: Thomas Güttler Owned by: nobody
Component: Documentation Version: 1.4
Severity: Normal Keywords:
Cc: ognajd@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Document best practice of Model Field Choices:

    class User(models.Model):
        GENDER_MALE = 0
        GENDER_FEMALE = 1
        GENDER_CHOICES = [(GENDER_MALE, 'Male'), (GENDER_FEMALE, 'Female')]
        gender = models.IntegerField(choices=GENDER_CHOICES)
    
        def greet(self):
            return {GENDER_MALE: 'Hi, boy', GENDER_FEMALE: 'Hi, girl.'}[self.gender]

related thread in django-devel:

https://groups.google.com/group/django-developers/browse_frm/thread/cf1e8b22d4906559/249e082b30ecdc14

Attachments (2)

ref_models_fields_choices.diff (862 bytes ) - added by Thomas Güttler 12 years ago.
fixed typo: in method self.GENDER_.. is needed
ref_models_fields_apr-13-2012.diff (1.3 KB ) - added by Daniel Sokolowski 12 years ago.
More direct to the point change

Download all attachments as: .zip

Change History (11)

by Thomas Güttler, 12 years ago

fixed typo: in method self.GENDER_.. is needed

comment:1 by Daniel Sokolowski, 12 years ago

Cc: ognajd@… added

What if one now wishes to create a custom manager that adds 'males' filter? You run into a circular import problem where you need to import from User but your user objects needs to assign attribute objects to your new UserManager. Potential solution that works for me is shown below. Albeit it is verbose it also is logical and makes sense; the UserManager object knows about the gender choices, the User just knows what gender it is.

class UserManager(models.Manager):
    GENDER_MALE = 0
    GENDER_FEMALE = 1
    GENDER_CHOICES = [(GENDER_MALE, 'Male'), (GENDER_FEMALE, 'Female')]
    def males(self):
        return self.all().filter(gender=self.GENDER_MALE)
    def females(self):
        return self.all().filter(gender=self.GENDER_FEMALE)

class User(models.Model):
    gender = models.IntegerField(choices=UserManager.GENDER_CHOICES)
    objects = UserManager()
    def greet(self):
        return {UserManager.GENDER_MALE: 'Hi, boy', UserManager.GENDER_FEMALE: 'Hi, girl.'}[self.gender]

comment:2 by Thomas Güttler, 12 years ago

Hi danols, thank you for looking at this ticket. You can put the GENDER_.... attributes on the User class without a circular import like this:

class UserManager(models.Manager):
    def males(self):
        return self.all().filter(gender=User.GENDER_MALE)
    def females(self):
        return self.all().filter(gender=User.GENDER_FEMALE)

class User(models.Model):
    GENDER_MALE = 0
    GENDER_FEMALE = 1
    GENDER_CHOICES = [(GENDER_MALE, 'Male'), (GENDER_FEMALE, 'Female')]
    gender = models.IntegerField(choices=GENDER_CHOICES)
    objects = UserManager()
    def greet(self):
        return {User.GENDER_MALE: 'Hi, boy', User.GENDER_FEMALE: 'Hi, girl.'}[self.gender]

by Daniel Sokolowski, 12 years ago

More direct to the point change

comment:3 by Daniel Sokolowski, 12 years ago

Triage Stage: UnreviewedDesign decision needed

Guetti you are absolutely right, thank you for correcting me!

In my opinion it still makes more sense to have the GENDER variables defined on the UserManager, however having the choices on the User model is much more convenient and most likely more common.

What do you think about making the changes much more concise as in the patch I added?

Daniel

comment:4 by Thomas Güttler, 12 years ago

Triage Stage: Design decision neededReady for checkin
Type: UncategorizedCleanup/optimization

@danols: about 2% of my models have a custom manager. About 20% have choices. I don't want to create a manager just to add the "GENDER" variables.

Yes, I like the solution in ref_models_fields_apr-13-2012.diff.

According to the ticke triage docs "desig decision needed" is for difficult stuff. I say "ready for commit" for ref_models_fields_apr-13-2012.diff.

comment:5 by Claude Paroz, 12 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Do not forget we are in a reference section here, so we shouldn't remove the example of defining choices outside the model. We should simply underline what is the best practice.

comment:6 by James Bennett <james@…>, 12 years ago

Resolution: fixed
Status: newclosed

In [7731cc8689b47ca83e988919b44bcec2c6728e4e]:

Fix #18062: Document best practices for choices in model fields.

comment:7 by James Bennett <james@…>, 12 years ago

In [b1517a310a0a9aa416bc479397d5a7b402be64e2]:

Merge pull request #256 from ubernostrum/model-choice-docs

Fix #18062: Document best practices for choices in model fields.

comment:8 by Thomas Güttler, 12 years ago

Cc: hv@… removed

Thank you James!

comment:9 by Simon Charette, 11 years ago

It looks like we should also update model coding styles to reflect this.

The part about choices contradicts best practices:

If choices is defined for a given model field, define the choices as a tuple of tuples, with an all-uppercase name, either near the 
top of the model module or just above the model class.
Note: See TracTickets for help on using tickets.
Back to Top