Code

Opened 2 years ago

Closed 2 years ago

Last modified 18 months ago

#18062 closed Cleanup/optimization (fixed)

Document Best Practice for Choices in Model Fields

Reported by: guettli 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 guettli 2 years ago.
fixed typo: in method self.GENDER_.. is needed
ref_models_fields_apr-13-2012.diff (1.3 KB) - added by danols 2 years ago.
More direct to the point change

Download all attachments as: .zip

Change History (11)

Changed 2 years ago by guettli

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

comment:1 Changed 2 years ago by danols

  • Cc ognajd@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 2 years ago by guettli

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 2 years ago by danols

More direct to the point change

comment:3 Changed 2 years ago by danols

  • Triage Stage changed from Unreviewed to Design 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 2 years ago by guettli

  • Triage Stage changed from Design decision needed to Ready for checkin
  • Type changed from Uncategorized to Cleanup/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 2 years ago by claudep

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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

  • Resolution set to fixed
  • Status changed from new to closed

In [7731cc8689b47ca83e988919b44bcec2c6728e4e]:

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

comment:7 Changed 2 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 2 years ago by guettli

  • Cc hv@… removed

Thank you James!

comment:9 Changed 18 months ago by charettes

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.