Code

Opened 7 years ago

Closed 7 years ago

#3946 closed (fixed)

Add Swiss localflavor

Reported by: charly.wilhelm@… Owned by: adrian
Component: contrib.localflavor Version: master
Severity: Keywords: localflavor, l10n, ch
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

Patch to add a module to django.contrib.localflavor for Switzerland: CHIdentityCardNumberField, CHPhoneNumberField, CHStateSelect and CHZipCodeField.

Attachments (2)

ch-localflavor.diff (5.2 KB) - added by charly.wilhelm@… 7 years ago.
initial ch localflavor
ch-localflavor2.diff (8.7 KB) - added by charly.wilhelm@… 7 years ago.
Replacement with the suggestions of mtredinnick

Download all attachments as: .zip

Change History (10)

Changed 7 years ago by charly.wilhelm@…

initial ch localflavor

comment:1 Changed 7 years ago by Simon G. <dev@…>

  • Has patch set
  • Keywords l10n, ch added; l10n removed
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Ready for checkin

Looks good - but a few tests would be nice, if possible?

comment:2 Changed 7 years ago by adrian

  • Component changed from Contrib apps to django.contrib.localflavor

comment:3 Changed 7 years ago by mtredinnick

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

Agree with Simon; some tests would be good just to exercise the code. They often catch bugs when/if we refactor anything. The page you used for the reference implementation of ID numbers has some examples at the end.

A few little improvements for the patch itself (all in forms.py)

  1. line 36 is a print statement. Make it go away!
  2. The validation error messages should be marked as translatable, since they are presented to the user (the person using the web browser). Since our translation functions cannot, at the moment, accept unicode strings, you'll have to remove the initial 'u' from those strings, but it doesn't look like that will hurt anything, since they appear to be ASCII strings currently anyway.
  3. line 67 turns "first" into a string, so why is str(first) needed on line 69? Also, line 69 can just be num = ord(first.upper()) - 65, which seems shorter.
  4. If you make the computation of num simpler on line 69, you can avoid assigning it to a variable and use it directly in line 70. Doesn't look like you're currently using num anywhere at the moment.
  5. It seems that if I pass in something that is not a digit in "number", the code is going to fail with an uncaught exception. You might want to be catching ValueError and failing the validation if that is raised. Again, tests will help exercise this (try an ID number of "banana", for example).
  6. line 91: you can't pass unicode strings to gettext() -- make-messages.py will complain. Not sure how to fix that yet (ticket #3923), so you have to work around it by not using unicode strings there.
  7. Best to say "Swiss-specific" in the docstring. Not everybody outside of Switzerland, particularly outside of Europe, associates CH with Switzerland.

That's all I spotted for the moment (it lengthy, but they're all little things).

comment:4 Changed 7 years ago by philipp.keller@…

Cool, there are other swiss guys using django!
I got a suggestion for the phone number field: I often write phone numbers "0xx/xxxxxxx" but the slash isn't supported by your parser.
I'd suggest the phonenumber parser to look for a pattern like

0([^0-9]*[0-9]){9}

This would allow also numbers 0xx-000-00-00.

Changed 7 years ago by charly.wilhelm@…

Replacement with the suggestions of mtredinnick

comment:5 Changed 7 years ago by charly.wilhelm@…

I changed the suggestions of mtredinnick and added some tests.

To Philipp Keller: Django is the best Framework I've ever seen. easy, fast and funny. To your sugguestion. Therefore I remove some non numeric characters (line 32) and parse the value with phone_digits (line 12), which I think must start with a 0, then a non 0, followed by 8 numbers.

This way the parser accepts also a number as 012/34-567.89 and cleans it to 012 345 67 89.

comment:6 Changed 7 years ago by Simon G. <dev@…>

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

comment:7 Changed 7 years ago by mtredinnick

In future, please make sure there are no subversion conflicts in your tree before generating the patch. You can see in the localflavor.py part of the patch, that there was a subversion conflict (the "<<<<", "====" and ">>>>" markers) that had to be manually fixed before I could commit this.

comment:8 Changed 7 years ago by mtredinnick

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

(In [5132]) Fixed #3946 -- Added Swiss localflavor. Thanks, charly.wilhelm@….

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.