Opened 15 years ago
Closed 11 years ago
#8273 closed Cleanup/optimization (wontfix)
Reduce amount of redundant code in django.contrib.localflavor
Reported by: | Owned by: | nobody | |
---|---|---|---|
Component: | contrib.localflavor | Version: | dev |
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)
Change History (11)
Changed 15 years ago by
Attachment: | localflavor-numberwithchecksumfield.patch added |
---|
comment:1 Changed 15 years ago by
comment:2 Changed 15 years ago by
milestone: | → post-1.0 |
---|---|
Triage Stage: | Unreviewed → 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 15 years ago by
Cc: | ubanus@… added |
---|
comment:5 Changed 13 years ago by
Severity: | → Normal |
---|---|
Type: | → Cleanup/optimization |
comment:6 Changed 13 years ago by
Easy pickings: | unset |
---|---|
Patch needs improvement: | set |
Patch needs improvement as per Malcolm's comment.
comment:7 Changed 13 years ago by
Cc: | jwilk@… added |
---|
comment:8 Changed 13 years ago by
Cc: | ubanus@… removed |
---|
comment:10 Changed 11 years ago by
Resolution: | → wontfix |
---|---|
Status: | new → 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.
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).