Code

Opened 4 years ago

Closed 3 years ago

#12595 closed Bug (fixed)

Localflavor bad arguments handling

Reported by: sayane Owned by: jezdez
Component: contrib.localflavor Version:
Severity: Normal Keywords:
Cc: say4ne@…, carbonXT Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

A lot of form fields in django.contrib.localflavor handles arguments to __init__() in a bad way.

    def __init__(self, *args, **kwargs):
        super(PLPESELField, self).__init__(r'^\d{11}$',
            max_length=None, min_length=None, *args, **kwargs)

Every line like first one should be fixed like this:

    def __init__(self, max_length=None, min_length=None, *args, **kwargs):

Because if someone will pass max_length or min_length argument to __init__() it will fail (multiple min_length/max_length arguments - additional one will be in kwargs).

It is a big problem, if i'm trying to do something like this:

class PostalCodeField(models.CharField):
    def formfield(self, **kwargs):
        kwargs.setdefault("form_class", PLPostalCodeField)
        return super(self.__class__, self).formfield(**kwargs)

because django.db.models.CharField sets max_length to self.max_length.

I'm attaching patch to fix polish localflavor.

Attachments (3)

localflavor_pl_fix.diff (1.6 KB) - added by sayane 4 years ago.
12595-1-wip.diff (5.6 KB) - added by claudep 3 years ago.
12595-2.diff (32.0 KB) - added by claudep 3 years ago.

Download all attachments as: .zip

Change History (15)

Changed 4 years ago by sayane

comment:1 Changed 4 years ago by sayane

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

comment:2 Changed 4 years ago by jezdez

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 4 years ago by jezdez

  • Owner changed from nobody to jezdez

comment:4 Changed 4 years ago by russellm

  • milestone 1.2 deleted
  • Version SVN deleted

comment:5 Changed 3 years ago by anonymous

  • Cc carbonXT added

comment:6 Changed 3 years ago by jezdez

  • Owner jezdez deleted

comment:7 Changed 3 years ago by mattmcc

  • Severity set to Normal
  • Type set to Bug

comment:8 Changed 3 years ago by julien

  • Needs tests set

comment:9 Changed 3 years ago by claudep

  • Easy pickings unset
  • Patch needs improvement set

I basically see what's the problem and what to do. Now there is a lot of similar changes to make, and I would like to know if I'm going in the right direction before giving more time to this.

See my patch, and tell me if it's OK (and good practice) to pass max_length and min_length to super as args (vs kwargs).
For tests, should I add field_kwargs to all tests, or add a second assert with field_kwargs to all tests, or simply add field_kwargs once per test case?

Changed 3 years ago by claudep

Changed 3 years ago by claudep

comment:10 Changed 3 years ago by claudep

  • Needs tests unset
  • Patch needs improvement unset

OK, fixed now in all localflavors. Tested in utils.assertFieldOutput.

comment:11 Changed 3 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:12 Changed 3 years ago by jezdez

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

In [16146]:

Fixed #12595 -- Fixed bad arguments handling in localflavor form fields.

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.