Code

Opened 6 years ago

Closed 21 months ago

#8273 closed Cleanup/optimization (wontfix)

Reduce amount of redundant code in django.contrib.localflavor

Reported by: Piotr Lewandowski <django@…> Owned by: nobody
Component: contrib.localflavor Version: master
Severity: Normal Keywords:
Cc: jwilk@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

There is a plenty of somehow redundant code in django.contrib.localflavor which leads to potentially more bugs and what is quite inconvenient -- many translation strings.

I would like to propose the patch that handles all fields with a checksum verification.

Pros:

  • a lot fewer translation strings, which is more consistent and means less work when translating Django to 50 languages,
  • DRY compatible (and all benefits that it implies).

Cons:

  • backwards incompatible,
  • new code may contain bugs (although all tests were OK).

I volunteer for further modifications of django.contrib.localflavor code to use NumberWithChecksumField if this patch is accepted.

Attachments (1)

localflavor-numberwithchecksumfield.patch (19.6 KB) - added by Piotr Lewandowski <django@…> 6 years ago.

Download all attachments as: .zip

Change History (11)

Changed 6 years ago by Piotr Lewandowski <django@…>

comment:1 Changed 6 years ago by Piotr Lewandowski <django@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I forgot to mention that this patch also fixes a bug in PLNationalBusinessRegisterField. This field should accept also 14-digit numbers (see http://www.stat.gov.pl/bip/389_117_ENG_HTML.htm for reference).

comment:2 Changed 6 years ago by mtredinnick

  • milestone set to post-1.0
  • Triage Stage changed from Unreviewed to Accepted

It should be possible to do this in a backwards-compatible fashion by declaring specific classes in each localflavor that are specific cases of a parameterised parent class. The intention here is good, since there is some quite apparent duplication, but right now isn't the time to be shuffling this code around. Let's table until after 1.0.

comment:3 Changed 6 years ago by Jakub Wilk <ubanus@…>

  • Cc ubanus@… added

comment:4 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:5 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:6 Changed 3 years ago by julien

  • Easy pickings unset
  • Patch needs improvement set

Patch needs improvement as per Malcolm's comment.

comment:7 Changed 3 years ago by jwilk

  • Cc jwilk@… added

comment:8 Changed 3 years ago by ubanus@…

  • Cc ubanus@… removed

comment:9 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 21 months ago by aaugustin

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

Localflavor has just been split in distinct repositories, making this refactoring significantly harder and less likely to happen.

Besides, I think that descriptive error messages are important for usability, even if they come at the cost of more translation strings.

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.