Deprecating Phone Fields #262

vladimirnani · 2016-11-06T22:17:06Z

Added deprecation for phone number fields in favor of django-phonenumber-field

timgraham

I didn't review this in complete detail, but I wanted to leave a few suggestions.

timgraham · 2016-11-10T18:15:13Z

localflavor/generic/forms.py

+class PhoneNumberFormFieldMixin(object):
+    def __init__(self):
+        super(PhoneNumberFormFieldMixin, self).__init__()
+        warnings.warn(


To be consistent with Django, I'd rather if you used from __future__ import unicode_literals rather than u prefixes (which are obsolete when dropping support for Python 2).

Django style is:

warnings.warn( "PhoneNumber form fields are deprecated in favor of the " "django-phonenumber-field library.", RemovedInLocalflavor20Warning, )

@timgraham I agree this is a good way to go. I'll update the docs that asks for u prefixed strings.
https://django-localflavor.readthedocs.io/en/latest/#adding-flavors

timgraham · 2016-11-10T18:16:26Z

README.rst

@@ -40,3 +40,19 @@ support for an unsupported version of Django.
 **2014-12-10 - 1.1**: Django 1.5 - 1.7

 **2013-07-29 - 1.0**: Django 1.5 - 1.6
+
+
+**Roadmap**


I would put this in the ocs rather than the README.

@benkonrath could you point me where Roadmap will fit better please

@vladimirnani You can put this info in the releases section since this roadmap is changing the release policy for localflavor.
https://github.com/django/django-localflavor/blob/master/docs/index.rst#releases

timgraham · 2016-11-10T18:16:53Z

docs/changelog.rst

@@ -38,6 +38,8 @@ Other changes:
  (`gh-257 <https://github.com/django/django-localflavor/pull/257>`_). Users will need to generate migrations for any
  model fields they use with 'makemigrations'.

+- Adapting django deprecation policy


This doesn't mean much to me. If you included the roadmap in the docs, you could link to it.

Also, include details of the deprecation in the docs.

timgraham · 2016-11-10T18:17:21Z

localflavor/au/models.py

@@ -49,6 +49,12 @@ class AUPhoneNumberField(CharField):
    """A model field that checks that the value is a valid Australian phone number (ten digits)."""

    description = _("Australian Phone number")
+    system_check_deprecated_details = {
+        'msg': (
+            "PhoneNumberFields has been deprecated.",


has been -> is

timgraham · 2016-11-10T18:19:15Z

tests/test_deprecated/tests.py

+
+class DeprecatedFieldsTests(SimpleTestCase):
+    def setUp(self):
+        logging.captureWarnings(True)


what warnings are being captured?

timgraham · 2016-11-10T18:19:34Z

tests/settings.py

@@ -16,6 +16,7 @@
    'tests.test_us',
    'tests.test_pk',
    'tests.test_generic',
+    'tests.test_deprecated'


Include a trailing comma so if more items are added to this list, we don't have to modify this line again.

vladimirnani · 2016-11-10T19:40:53Z

@timgraham thanks for feedback will fix it shortly

vladimirnani · 2016-11-10T20:45:15Z

@timgraham btw maybe you know why my test fails on Django 1.7

benkonrath · 2016-11-10T21:15:40Z

Sorry that i haven't had time to review this yet. Thanks for the first review Tim.

@vladimirnani The base class should be at the far right and the mixins to the left. That might be the cause of the test failures.

timgraham · 2016-11-10T21:30:32Z

The test failure is because system_check_deprecated_details is new in Django 1.8. If it were up to me, i would drop support for Django 1.7 at this point as it's unsupported by the Django team.

benkonrath · 2016-11-10T22:07:36Z

I'm fine with dropping support for Django 1.7. We only kept it around because it wasn't causing problems. I've reopened #218 for this and will merge it shortly.

benkonrath · 2016-11-10T22:12:41Z

@vladimirnani Please rebase your branch to get the version without the Django 1.7 tests.

vladimirnani · 2016-11-11T19:37:45Z

@benkonrath Ok. Done.

benkonrath · 2016-11-26T09:23:30Z

@vladimirnani Just trying to get up to speed on this PR. Why are you using the Python warnings system for the form field deprecations instead to the Django system check framework? I can see that there's built-in support for deprecating model fields with the system check framework so it makes sense to use it. Couldn't we also add similar support (in localflavor) for deprecating form fields with the system check framework?

Is there something "wrong" with using the Django system check framework for the form field depredations? Or is this just about Django conventions for deprecations of non-model fields?

@timgraham Perhaps you have an idea about this?

benkonrath · 2016-11-26T09:26:41Z

Thinking out loud ... I suppose one of the benefits to using the warnings system is that we could use the deprecation easily in other parts of localflavor. I'm still curious to understand the reasoning on the setup in the PR. Thanks.

timgraham · 2016-11-26T10:58:01Z

From a quick look, this approach looks like what we would do with Django, always using warnings for deprecations, except for deprecation of model fields.

benkonrath · 2016-11-26T11:51:39Z

Thanks TIm. Let's stick with the Django approach for this and use Python warnings.

vladimirnani · 2016-11-26T13:05:07Z

Yes I have checked how it is being done in Django. So for now I think we can stick with warnings for everything except model fields.

benkonrath

@vladimirnani I've made some comments for some improvements. Thanks again for working on this.

benkonrath · 2016-11-26T19:24:45Z

docs/index.rst

+Roadmap
+-------
+
+django-localflavor releases follows `semver`_.


follows -> follow

benkonrath · 2016-11-26T19:25:11Z

docs/index.rst

+
+django-localflavor releases follows `semver`_.
+Within one month of django release we would release a new verision.
+We might have an extra release if there is lots features in between django releases.


if there is lots -> if there are enough

benkonrath · 2016-11-26T19:30:37Z

docs/index.rst

+1.4          November 2016
+1.x          ...
+2.0          January 2018
+=========== ===============


Do you think it's worth specifying the corresponding Django release? We know that localflavor 2.0 will correspond with Django 2.0 and that localflavor 1.4 is the (late) version for Django 1.10. We're not trying to match Django versions but having the matching Django version does highlight our "within one month" release policy. Anyway, just an idea. Add it only if you agree and think it makes things more clear.

benkonrath · 2016-11-26T19:41:31Z

tests/test_deprecated/tests.py

+    @override_settings(SILENCED_SYSTEM_CHECKS=[])
+    def test_PhoneNumberField_deprecated(self):
+        class PhoneNumberModel(Model):
+            phone_number = models.NLPhoneNumberField()


Could you test all deprecated model fields? I think you can add a field for all of the phone number model fields to the same model. Thanks.

benkonrath · 2016-11-26T20:06:59Z

localflavor/generic/forms.py

@@ -127,3 +132,13 @@ def prepare_value(self, value):
        if value is not None:
            return value.upper()
        return value
+
+
+class PhoneNumberFormFieldMixin(object):


It would be nice to have the word "Deprecation" or "Deprecated" in the class name. I realize that it makes it a pretty long name, but it helps to immediately know what it does.

benkonrath · 2016-11-26T20:29:04Z

localflavor/generic/forms.py

+        super(PhoneNumberFormFieldMixin, self).__init__()
+        warnings.warn(
+            "PhoneNumber form fields are deprecated in favor of the "
+            "django-phonenumber-field library.",


What about using self.__class__.__name__ instead of referring generically to "PhoneNumber form fields". I think it would help make things clearer.

benkonrath · 2016-11-26T20:49:12Z

localflavor/pk/models.py

@@ -48,6 +48,12 @@ class PKPhoneNumberField(CharField):
    """A model field that checks that the value is a valid Pakistani phone number (nine to eleven digits)."""

    description = _("Pakistani Phone number")
+    system_check_deprecated_details = {
+        'msg': (
+            "PhoneNumberFields are deprecated.",


Can you use specific field name in the msg for all of the model fields? That will make things more clear for users seeing this message.

benkonrath · 2016-11-26T20:52:29Z

tests/test_deprecated/tests.py

+from localflavor.nl import models
+
+
+class DeprecatedFieldsTests(SimpleTestCase):


Can you test the form field deprecations as well? There are examples in the Django tests for this.

benkonrath · 2016-11-26T21:40:05Z

One last question: DeprecationWarning and PendingDeprecationWarning are ignored by default in Python. I see the DeprecationWarnings on the console from Django when code uses something that will be removed. How can localflavor also have the DeprecationWarnings printed on the console? I tried this PR in an existing Django 1.10 / Python 3.4 project with the PendingDeprecationWarning changed to DeprecationWarning and didn't see the warnings in the console.

timgraham · 2016-11-26T21:59:16Z

Actually, Django 1.11 removes the "Deprecation warnings loud by default" behavior.

benkonrath · 2016-11-27T10:14:40Z

I guess that makes it easier for our release but less friendly for users. There's also an inconsistency here: deprecations on model fields will be loud because they use system check, deprecations on form fields will be silent unless a user takes a manual step to enable them. This seems like a good argument to use system check for all deprecations in localflavor. I don't want to expand the scope of this issue so I think we should continue as with the Python warnings as implemented in this PR. We can think about changing this later - perhaps with some discussion about this on django-devel.

timgraham · 2016-11-27T10:33:45Z

Unlike models, all forms aren't necessarily imported when the system check frameworks runs which is why it's infeasible to use it for form checks.

benkonrath · 2016-11-27T11:20:13Z

Ok, got it. That's why system checks isn't used for form field deprecation in Django. Thanks.

benkonrath · 2016-12-05T11:58:07Z

@vladimirnani Do you think you'll have time to update the PR soon? I'd like to get the 1.4 release out soon and this issue should be included in 1.4. If you're too busy, I can take over the PR but I'd like to then get your input on my changes which means I'd like you to review my PR. Let me know what is best for you. Thanks.

vladimirnani · 2016-12-06T07:13:16Z

@benkonrath I will find time tomorrow to update it.

vladimirnani · 2016-12-08T20:14:14Z

@benkonrath just found that there is CNCellNumberField alongside with CNPhoneNumberField That one also should be deprecated.

benkonrath · 2016-12-18T09:36:03Z

Thanks @vladimirnani!

timgraham reviewed Nov 10, 2016

View reviewed changes

benkonrath mentioned this pull request Nov 10, 2016

Drop support for django 1.7. #218

Merged

benkonrath mentioned this pull request Nov 10, 2016

WIP: Deprecate all phonenumber validators and model fields. #223

Closed

vladimirnani force-pushed the deprecation branch 3 times, most recently from 90b5e61 to 7f5028b Compare November 11, 2016 07:51

vladimirnani force-pushed the deprecation branch from 7f5028b to 5132306 Compare November 11, 2016 22:06

benkonrath mentioned this pull request Nov 12, 2016

Refactored complex methods #263

Merged

benkonrath mentioned this pull request Nov 26, 2016

Change 'DIF' to 'CDMX' in mx_states.py #234

Closed

benkonrath requested changes Nov 26, 2016

View reviewed changes

vladimirnani force-pushed the deprecation branch from 5132306 to bf23578 Compare December 8, 2016 21:13

Deprecating Phone Fields

4f954ba

vladimirnani force-pushed the deprecation branch from bf23578 to 4f954ba Compare December 8, 2016 21:20

benkonrath approved these changes Dec 18, 2016

View reviewed changes

benkonrath merged commit c078000 into django:master Dec 18, 2016

benkonrath mentioned this pull request Dec 18, 2016

Update docs #271

Merged

This was referenced Jan 20, 2017

Python dependencies are outdated savoirfairelinux/sous-chef#632

Closed

CAPhoneNumberField: RemovedInLocalflavor20Warning savoirfairelinux/sous-chef#634

Open

		from localflavor.nl import models


		class DeprecatedFieldsTests(SimpleTestCase):

Deprecating Phone Fields #262

Deprecating Phone Fields #262

Conversation

vladimirnani commented Nov 6, 2016

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benkonrath Nov 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirnani commented Nov 10, 2016

vladimirnani commented Nov 10, 2016

benkonrath commented Nov 10, 2016 • edited Loading

timgraham commented Nov 10, 2016

benkonrath commented Nov 10, 2016

benkonrath commented Nov 10, 2016

vladimirnani commented Nov 11, 2016

benkonrath commented Nov 26, 2016

benkonrath commented Nov 26, 2016

timgraham commented Nov 26, 2016

benkonrath commented Nov 26, 2016

vladimirnani commented Nov 26, 2016

benkonrath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benkonrath commented Nov 26, 2016

timgraham commented Nov 26, 2016

benkonrath commented Nov 27, 2016

timgraham commented Nov 27, 2016

benkonrath commented Nov 27, 2016

benkonrath commented Dec 5, 2016

vladimirnani commented Dec 6, 2016

vladimirnani commented Dec 8, 2016

benkonrath commented Dec 18, 2016

benkonrath Nov 11, 2016 •

edited

Loading

benkonrath commented Nov 10, 2016 •

edited

Loading