Opened 3 years ago

Closed 3 years ago

#17864 closed New feature (fixed)

Add localflavor for Hong Kong, starting with a phone number field

Reported by: mrkschan@… Owned by: mrkschan
Component: contrib.localflavor Version: 1.3
Severity: Normal Keywords: localflavor hk
Cc: Fandekasp Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Hong Kong, ISO 3166 country code hk (http://is.gd/RIte7H), needs a local flavor for her phone number.

Attachments (4)

django.contrib.localflavor.hk.diff (3.2 KB) - added by mrkschan 3 years ago.
git-diff
hkphonenumber.2.diff (5.9 KB) - added by Fandekasp 3 years ago.
Field fixed + Tests + translatable strings + Doc updated
hkphonenumber.diff (5.9 KB) - added by Fandekasp 3 years ago.
Field fixed + Tests + translatable strings + Doc updated
hkphonenumber.3.diff (6.5 KB) - added by Fandekasp 3 years ago.
Fixed with complete test + doc

Download all attachments as: .zip

Change History (16)

comment:1 Changed 3 years ago by mrkschan

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to mrkschan
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 follow-up: Changed 3 years ago by aaugustin

  • Easy pickings set
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Summary changed from Add Hong Kong phone number to django.contrib.localflavor.hk to Add localflavor for Hong Kong, starting with a phone number field
  • Triage Stage changed from Unreviewed to Accepted

I've rephrased the title to make it clearer that this patch adds a new country to localflavor.

We generally don't include references to South in Django, that comment should be removed.

This needs tests and docs, but they should be very easy to write, using other countries as examples.

comment:3 in reply to: ↑ 2 Changed 3 years ago by mrkschan

Replying to aaugustin:

I've rephrased the title to make it clearer that this patch adds a new country to localflavor.

We generally don't include references to South in Django, that comment should be removed.

This needs tests and docs, but they should be very easy to write, using other countries as examples.

I find no test under django/contrib/localflavor/ in the source distribution tarball (http://www.djangoproject.com/download/1.3.1/tarball/), am I looking at the right place?

Last edited 3 years ago by mrkschan (previous) (diff)

comment:4 Changed 3 years ago by aaugustin

Changed 3 years ago by mrkschan

git-diff

comment:5 Changed 3 years ago by mrkschan

I didn't have experience in writing proper django style docstrings. Could you give me hints? See attached the new patch.

comment:6 Changed 3 years ago by Fandekasp

mrkschan, look at https://docs.djangoproject.com/en/1.3/internals/documentation/ for django-specific markup

Can't see any test in the diff as well

All strings should be translatable, you need ugettext_lazy for the default_error_messages

You should remove your localflavor/hk/models.py file, that doesn't make sense. A 'PhoneNumberField' field already exists in localflavor/us/models.py

missing init.py in localflavor/hk/

forms.py should better starts with "from future import absolute_import"

missing doc

Last edited 3 years ago by Fandekasp (previous) (diff)

comment:7 Changed 3 years ago by Fandekasp

  • Needs tests unset
  • Patch needs improvement unset

Changed 3 years ago by Fandekasp

Field fixed + Tests + translatable strings + Doc updated

Changed 3 years ago by Fandekasp

Field fixed + Tests + translatable strings + Doc updated

comment:8 follow-up: Changed 3 years ago by Fandekasp

  • Cc Fandekasp added
  • Needs documentation unset

comment:9 Changed 3 years ago by julien

  • Triage Stage changed from Accepted to Ready for checkin

Thanks!

comment:10 in reply to: ↑ 8 Changed 3 years ago by mrkschan

Replying to Fandekasp:

Missed a valid testcase -

'85291111111': '9111-1111',

Missed a invalid testcase -

'2111--1111': [error_msgs['invalid'], ]

i have prepared a set of comprehensive test cases - see http://is.gd/QE13Zs

Changed 3 years ago by Fandekasp

Fixed with complete test + doc

comment:11 Changed 3 years ago by Fandekasp

Done, glad to be of help.

I also modified the invalid error message to inform about all acceptable inputs.

comment:12 Changed 3 years ago by julien

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

In [17886]:

Fixed #17864 -- Added Hong Kong localflavor. Thanks to mrkschan and Adrien Lemaire.

Note: See TracTickets for help on using tickets.
Back to Top