Opened 5 years ago

Closed 4 years ago

Last modified 4 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 5 years ago.
fixed typo: in method self.GENDER_.. is needed
ref_models_fields_apr-13-2012.diff (1.3 KB) - added by Daniel Sokolowski 5 years ago.
More direct to the point change

Download all attachments as: .zip

Change History (11)

Changed 5 years ago by Thomas Güttler

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

comment:1 Changed 5 years ago by Daniel Sokolowski

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 Changed 5 years ago by Thomas Güttler

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]

Changed 5 years ago by Daniel Sokolowski

More direct to the point change

comment:3 Changed 5 years ago by Daniel Sokolowski

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 Changed 5 years ago by Thomas Güttler

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 Changed 5 years ago by Claude Paroz

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 Changed 4 years ago by James Bennett <james@…>

Resolution: fixed
Status: newclosed

In [7731cc8689b47ca83e988919b44bcec2c6728e4e]:

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

comment:7 Changed 4 years ago by James Bennett <james@…>

In [b1517a310a0a9aa416bc479397d5a7b402be64e2]:

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

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

comment:8 Changed 4 years ago by Thomas Güttler

Cc: hv@… removed

Thank you James!

comment:9 Changed 4 years ago by Simon Charette

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