Adding a INPhoneNumberField to indian localflavor

[Kenneth Gonsalves]

Indian landline phone numbers have an STD code prefixed by a '0' and followed by 2-4 digits. Followed by a 7 digit number that starts with the numbers 1-8. The two are separated by either a '-' or a space.

[Kenneth Gonsalves]

diff between current forms.py and proposed forms.py

[Kenneth Gonsalves]

After extensive discussion in Indian python circles it was found that the patch submitted is not fully accurate. A fresh regex has been formulated and tests are being rewritten, so this ticket is not yet ready for review.

[Kenneth Gonsalves]

ok, now all is done. The documentation explains the format. Two files attached - the diff on django/contrib/localflavor/in_/forms.py and in.py which contains the tests. (why does the in directory have a trailing underscore?)

[Kenneth Gonsalves]

in.py with tests for INPhoneNumberField

[Kenneth Gonsalves]

diff between current forms.py and proposed forms.py

[Julien Phalip]

Thanks for this suggestion. Could you write some tests for this, either in source:django/trunk/tests/regressiontests/localflavor or source:django/trunk/tests/regressiontests/forms/localflavor ?

[Kenneth Gonsalves]

tests are there in 'in.py' uploaded 7 weeks back. I have unchecked the 'needs tests' field.

[Julien Phalip]

OK I see. Could you rewrite your tests using unittest, and also combine all changes within one single diff patch? Thanks!

[Kenneth Gonsalves]

you want unittests in addition to doctests? or instead of doctests? I have the unittests, but since I could not find unittests for other phonenumber flavors, I did not put them.

[Russell Keith-Magee]

One of the features of 1.3 is that we replaced almost all the doctests in Django with unit tests. Going forward, we're only adding unit tests.

So - any patch being proposed for trunk needs to include unittests not doctests.

[Julien Phalip]

You may add your unittests either in source:django/trunk/tests/regressiontests/localflavor or source:django/trunk/tests/regressiontests/forms/localflavor , whichever makes more sense to you ;)

[Kenneth Gonsalves]

ok - will do - good thing you are removing doctests - they are a pain to write.

[Kenneth Gonsalves]

all ready, but I am confused as how to combine everything in one patch. svn diff gives me the changes in forms.py. I have also created an in.py in ​http://code.djangoproject.com/browser/django/trunk/tests/regressiontests/forms/localflavor - how do I add this to the patch?

[Julien Phalip]

My svn skills are a bit rusty but I think you need to 'svn add' the new file before running diff. I've also found this: ​http://stackoverflow.com/questions/4248768/including-new-files-in-svn-diff

If you're still struggling, I suggest you ask for help on the django-users list. Thank you!

[Kenneth Gonsalves]

cool - I haven't used svn for some years now. svn add did the trick. I will just test everything out and upload tomorrow. Thanks for you patience

[Kenneth Gonsalves]

attached the patch with tests

[Alex Gaynor]

A ticket isn't closed when a patch is uploaded.

[Kenneth Gonsalves]

apologies - my first time

[Julien Phalip]

Thank you for your own patience, lawgon. Could you also please write some documentation for this new feature? :)

[Kenneth Gonsalves]

class in_.forms.INPhoneNumberField validates that the data is a valid Indian phone number, including the STD code. It's normalised to 0XXX-XXXXXXX or 0XXX XXXXXXX format. The first string is the STD code which is a '0' followed by 2-4 digits. The second string is 8 digits if the STD code is 3 digits, 7 digits if the STD code is 4 digits and 6 digits if the STD code is 5 digits. The second string will start with numbers between 1 and 6. The separator is either a space or a hyphen.

This is the documentation (I had put it in the doctests file which has now been left out) - It is now in the docstring. Will upload the latest patch.

[Julien Phalip]

Sorry, I should have been clearer. You need to write the doc in source:django/trunk/docs/ref/contrib/localflavor.txt so that it appears in ​http://docs.djangoproject.com/en/dev/ref/contrib/localflavor/#india-in

By the way, what contribution did "Steve Fernandez" do in this patch? You may add his name and yours -- or maybe YOU are Steve :-) -- in source:django/trunk/AUTHORS if you like.

[Kenneth Gonsalves]

documentation added. Steve removed. Fresh patch attached. Changed type in docs - the directory is 'in_' and not 'in' (wonder why?)

[Julien Phalip]

Actually hooked up tests

[Julien Phalip]

I tried your patch was the test wasn't run because it wasn't actually hooked up to the test suite. I've fixed that in the attached patch by updating tests/regressiontests/forms/localflavortests.py and tests/regressiontests/forms/tests/__init__.py

Now, the test can be run but it raises this error:

======================================================================
ERROR: test_INPhoneNumberField (regressiontests.forms.localflavor.in_.INLocalFlavorTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/julien/.virtualenvs/djangotests/src/django/tests/regressiontests/forms/localflavor/in_.py", line 24, in test_INPhoneNumberField
    self.assertFieldOutput(INPhoneNumberField, valid, invalid)
  File "/Users/julien/.virtualenvs/djangotests/src/django/tests/regressiontests/forms/localflavor/utils.py", line 41, in assertFieldOutput
    required.clean, input
  File "/Users/julien/.virtualenvs/djangotests/src/django/django/utils/unittest/case.py", line 991, in assertRaisesRegexp
    expected_regexp = re.compile(expected_regexp)
  File "/opt/local/lib/python2.5/re.py", line 188, in compile
    return _compile(pattern, flags)
  File "/opt/local/lib/python2.5/re.py", line 241, in _compile
    raise error, v # invalid expression
error: bad character range

----------------------------------------------------------------------

Needs fixing :)

I'm not sure if that's the cause, but tests need to pass on Python 2.5.

[Russell Keith-Magee]

@lawgon -- the directory is called in_ because "in" is a keyword in Python.

[Kenneth Gonsalves]

I have only tested on 2.7 - will check on 2.6 and 2.5 and revert. Nice to know that our country has been honoured with a python keyword

[Kenneth Gonsalves]

it fails in 2.6 also. However the unit tests of the re expression pass in 2.6. I will investigate.

[Kenneth Gonsalves]

I am unable to fix this. Import of INPhoneNumberField into a form works as advertised. I also tried with a single line re (without VERBOSE) but the same re error comes up in the tests. Finally I tried with a very simple re - r'\d*'. This works. My conclusion is that in testing, an re beyond a certain length (or maybe beyond a certain level of complexity) will cause a syntax error although the same works in django itself. This is beyond my competence to correct. Should I file a bug?

[Kenneth Gonsalves]

I now tried with a simple re containing the '\d' repeated 85 times (this makes it the same length of my re). The tests accept it. So it must lie in the complexity.

[Julien Phalip]

Wow, it took me a long time to track this down. In fact, there's nothing really wrong with your patch. I think the problem is in the way that django.tests.regressiontests.forms.localflavor.utils.assertFieldOutput works.

assertFieldOutput calls assertRaisesRegexp and passes the expected errors as a unicode string:

In source:django/trunk/tests/regressiontests/forms/localflavor/utils.py#L40 :

self.assertRaisesRegexp(ValidationError, unicode(errors),
                required.clean, input
            )

... and further down the track, in assertRaisesRegexp, that string is compiled as a regular expression:

In source:django/trunk/django/utils/unittest/case.py#L991 :

expected_regexp = re.compile(expected_regexp)

Now, the issue here is that the error message in your patch contains some '-' characters, which are interpreted as a range, hence the 'bad character range' error that is raised when running the tests:

class INLocalFlavorTests(LocalFlavorTestCase):
    def test_INPhoneNumberField(self):
        error_format = [u'Phone numbers must be in 02X-7X or 03X-6X or 04X-5X format.']
    ...

Remove those '-' characters and the tests will pass... Now of course, this is not satisfactory. I tend to think it's a bug in the test framework. Let me know if somebody has an opinion on this, otherwise I might bring this up on django-dev.

[Kenneth Gonsalves]

Given that the framework itself does not barf when the patch is used, I think that this bug needs to be fixed. The world will not end if django does not yet have an Indian Phone number field, but I have a few more of this type of thing in the Queue and some of them also use '-' in the error messages. So please take it up on the dev list. Frankly I do not think that error messages should compile as regex.

[Julien Phalip]

I think I've found the root cause for this problem and posted a proposed fix in #15805. If that fix gets checked in then your patch will be ready for checkin.

[Kenneth Gonsalves]

found a typo in my patch. Attached a fresh patch.

[Kenneth Gonsalves]

fresh patch that fixes typo in the error message

[Julien Phalip]

Thanks for the update. By the way, feel free to add your name to the AUTHORS file, if only in memory of this epic ticket :)

[Kenneth Gonsalves]

as #15805 is fixed, can this be checked in?

[Julien Phalip]

This one can easily proceed as soon as #15813 gets checked in. I'm keeping an eye on it ;)

[Julien Phalip]

Patch updated to latest trunk

[Julien Phalip]

Good to go, thanks!

[Jannis Leidel]

In [16495]:

Fixed #14608 -- Added phone number field to Indian local flavor. Thanks, lawgon and Julien Phalip.