Added local-flavor support for Cuba #292

aaboffill · 2017-06-02T16:20:08Z

All Changes

Add an entry to the docs/changelog.rst describing the change.
Add an entry for your name in the docs/authors.rst file if it's not
already there.
Adjust your imports to a standard form by running this command:
```
`isort --recursive --line-width 120 localflavor tests`
```

New Fields Only

Prefix the country code to all fields.
Field names should be easily understood by developers from the target
localflavor country. This means that English translations are usually
not the best name unless it's for something standard like postal code,
tax / VAT ID etc.
Prefer 'PostalCodeField' for postal codes as it's
international English; ZipCode is a term specific to the United
States postal system.
Add meaningful tests. 100% test coverage is not required but all
validation edge cases should be covered.
Add .. versionadded:: <next-version> comment markers to new
localflavors.
Add documentation for all fields.

benkonrath · 2017-06-06T09:45:30Z

Thanks for this pull request. You can rebase with master to fix the failing test. I made a mistake removing the Python 3.4 / master test config in Travis but it should be fixed now.

Let me know when this is ready for a review.

codecov-io · 2017-06-06T13:08:36Z

Codecov Report

Merging #292 into master will decrease coverage by 0.03%.
The diff coverage is 95.04%.

@@            Coverage Diff             @@
##           master     #292      +/-   ##
==========================================
- Coverage   96.28%   96.25%   -0.04%     
==========================================
  Files         152      156       +4     
  Lines        4230     4351     +121     
  Branches      579      585       +6     
==========================================
+ Hits         4073     4188     +115     
- Misses         98      102       +4     
- Partials       59       61       +2

Impacted Files	Coverage Δ
localflavor/cu/models.py	`100% <100%> (ø)`
localflavor/cu/choices.py	`100% <100%> (ø)`
localflavor/cu/validators.py	`76.47% <76.47%> (ø)`
localflavor/cu/forms.py	`96.29% <96.29%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 067ed1c...59f17a4. Read the comment docs.

aaboffill · 2017-06-06T13:20:50Z

Hi! @benkonrath , thanks for your suggestion. I made the rebase and finally all checks have passed. This PR is ready for review. In few days i'll take some time to ensure 100% of the coverage of localflavor/cu/forms.py.

benkonrath · 2017-06-06T14:32:58Z

I noticed that some of my commits are now included in this PR which probably isn't what you were intending. You should do a git rebase against the latest version from master to replay your commits on top of the latest version from master. Since you're rewriting the history of your branch, you'll have to force push your branch. While you do the rebase, you can also squash the commits into 1 commit if you want.

Here's a guide I found that might be helpful:
https://help.github.com/articles/about-git-rebase/

aaboffill · 2017-06-07T15:45:54Z

ohh! yes I see, thanks for the suggestion, I'll try ti fix that. Thanks for the suggestion @benkonrath ...

Python 3.2 is no longer supported for Django 1.8 and above.

The next version of Django (2.0) will not support Python 3.4.

six has been removed since the version from django.utils is used in localflavor.

aaboffill · 2017-06-07T16:23:07Z

Hi! @benkonrath I organized the commits.

benkonrath · 2017-06-30T10:25:34Z

Sorry for the delay in getting to this. I'll be able to do a review early next week.

benkonrath

Thanks again for this. There are a couple of changes needed then it be good go.

benkonrath · 2017-07-04T17:46:33Z

docs/changelog.rst

@@ -6,7 +6,8 @@ Changelog

 New flavors:

- None
+- Added local flavor for Ukraine


Ukraine -> Cuba

benkonrath · 2017-07-04T17:53:31Z

localflavor/cu/forms.py

+        return value.strip()
+
+
+class CUPhoneNumberField(RegexField):


localflavor has deprecated all phone number fields in favour of https://github.com/stefanfoulis/django-phonenumber-field. The CUPhoneNumberField form field shouldn't be included in this PR.

benkonrath · 2017-07-04T17:55:33Z

localflavor/cu/models.py

+        return super(CUIdentityCardNumberField, self).formfield(**defaults)
+
+
+class CUPhoneNumberField(CharField):


The CUPhoneNumberField should also not be included.

benkonrath · 2017-07-04T21:18:39Z

localflavor/cu/forms.py

+
+    def clean(self, value):
+        super(CURegionField, self).clean(value)
+        if value in EMPTY_VALUES:


EMPTY_VALUES -> self.empty_values

The same applies to CUProvinceField

benkonrath · 2017-07-04T21:20:53Z

localflavor/cu/forms.py

+            return ''
+        try:
+            value = value.strip().lower()
+        except AttributeError:


As far as I can see you don't need to catch an AttributeError here.

The same applies to CUProvinceField

benkonrath · 2017-07-04T21:29:26Z

localflavor/cu/forms.py

+        super(CUPostalCodeField, self).__init__(r'^[1-9]\d{4}$', *args, **kwargs)
+
+    def to_python(self, value):
+        value = super(CUPostalCodeField, self).to_python(value)


You need to add localflavor.compat.EmptyValueCompatMixin to this class and add this python code:

if value in self.empty_values: return value

This comes from a recent change that was made to properly support Django >= 1.11 (#298). You can search through the other localflavor modules to see examples of how to set this up if you're not sure.

benkonrath · 2017-07-04T21:31:02Z

localflavor/cu/forms.py

+
+    def to_python(self, value):
+        value = super(CUIdentityCardNumberField, self).to_python(value)
+        return value.strip()


This also needs to use the EmptyValueCompatMixinas explained above.

benkonrath · 2017-07-04T21:52:18Z

localflavor/cu/forms.py

+    def __init__(self, *args, **kwargs):
+        super(CUIdentityCardNumberField, self).__init__(r'^\d{11}$', *args, **kwargs)
+
+    def _set_regex(self, regex):


You shouldn't override _set_regex. Either use CharField and set the CUIdentityCardNumberValidator using the validators arg in __init__() or use the RegexField like you do now and check the date in clean() after calling super instead of using the CUIdentityCardNumberValidator.

benkonrath · 2017-07-04T21:55:24Z

localflavor/cu/models.py

+        return name, path, args, kwargs
+
+
+class CUZipCodeField(CharField):


ZipCode -> Postal Code

The description also needs to be changed.

aaboffill · 2017-08-11T19:11:05Z

Hi @benkonrath , sorry for the delay. I updated the code with all your suggestions. Thanks for your time :) , if you have another question/suggestion, please let me know.

benkonrath · 2017-08-16T20:23:39Z

@aaboffill Thanks for the update - it looks good. One some improvement that I missed last time: 'cuban' should be changed to 'Cuban' in the docstrings because it's a proper noun. Once that's updated, I'll merge this.

aaboffill · 2017-08-29T17:34:54Z

Hi! @benkonrath , thanks for your review :) ... I made your last suggestion related to Cuban instead of cuban

benkonrath · 2017-09-02T20:18:25Z

Merged via 328caad.

Thanks!

benkonrath and others added 12 commits June 7, 2017 12:15

Removed tests using Python 3.2.

71e8a13

Python 3.2 is no longer supported for Django 1.8 and above.

Removed tests for Django master using Python 3.4.

8721622

The next version of Django (2.0) will not support Python 3.4.

Updated test requirements.

19c864c

six has been removed since the version from django.utils is used in localflavor.

Removed Python 3.4 / master test from Travis config.

bcddd8f

Added local-flavor support for Cuba

a0a7a4e

Added versionadded reference

fad54fd

Adjusted the imports to a standard form by command

3fbf6bc

Added entries for authors.rst and changelog.rst

669c536

Fixed tests

d2b356c

Changed CUZipCodeField by CUPostalCodeField

e5c290b

Fixed isort issue

b3bc5c4

Fixed some isort issues

d4ff9a0

aaboffill force-pushed the localflavor_for_cu branch from eb80be1 to d4ff9a0 Compare June 7, 2017 16:16

benkonrath requested changes Jul 4, 2017

View reviewed changes

Adonys Alea Boffill added 3 commits August 11, 2017 10:56

Merge remote-tracking branch 'upstream/master' into localflavor_for_cu

4581a3d

Added PR suggestions to merge local flavor for cuba

24dedb5

Fixed isort issue

031d866

Improved some docstrings and messages

59f17a4

aaboffill force-pushed the localflavor_for_cu branch from 01847d5 to 59f17a4 Compare August 29, 2017 17:26

benkonrath approved these changes Sep 2, 2017

View reviewed changes

benkonrath closed this Sep 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added local-flavor support for Cuba #292

Added local-flavor support for Cuba #292

aaboffill commented Jun 2, 2017 •

edited

Loading

benkonrath commented Jun 6, 2017

codecov-io commented Jun 6, 2017 •

edited

Loading

aaboffill commented Jun 6, 2017

benkonrath commented Jun 6, 2017

aaboffill commented Jun 7, 2017

aaboffill commented Jun 7, 2017

benkonrath commented Jun 30, 2017

benkonrath left a comment

benkonrath Jul 4, 2017

benkonrath Jul 4, 2017

benkonrath Jul 4, 2017

benkonrath Jul 4, 2017

benkonrath Jul 4, 2017

benkonrath Jul 4, 2017

benkonrath Jul 4, 2017

benkonrath Jul 4, 2017

benkonrath Jul 4, 2017

aaboffill commented Aug 11, 2017 •

edited

Loading

benkonrath commented Aug 16, 2017

aaboffill commented Aug 29, 2017

benkonrath commented Sep 2, 2017

		return super(CUIdentityCardNumberField, self).formfield(**defaults)


		class CUPhoneNumberField(CharField):

		return name, path, args, kwargs


		class CUZipCodeField(CharField):

Added local-flavor support for Cuba #292

Added local-flavor support for Cuba #292

Conversation

aaboffill commented Jun 2, 2017 • edited Loading

benkonrath commented Jun 6, 2017

codecov-io commented Jun 6, 2017 • edited Loading

Codecov Report

aaboffill commented Jun 6, 2017

benkonrath commented Jun 6, 2017

aaboffill commented Jun 7, 2017

aaboffill commented Jun 7, 2017

benkonrath commented Jun 30, 2017

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

Choose a reason for hiding this comment

aaboffill commented Aug 11, 2017 • edited Loading

benkonrath commented Aug 16, 2017

aaboffill commented Aug 29, 2017

benkonrath commented Sep 2, 2017

aaboffill commented Jun 2, 2017 •

edited

Loading

codecov-io commented Jun 6, 2017 •

edited

Loading

aaboffill commented Aug 11, 2017 •

edited

Loading