Clean up Dutch local flavor. #152

jieter · 2015-05-09T12:54:20Z

This is my initial attempt on #150. As of now, nothing is (or at least should be) renamed or deprecated. This is what I did:

Move validators to validators.py
Add model fields for existing form fields
Add form fields for existing model fields
Move tests to tests/test_nl/
Increase test coverage.
Tried to make the error messages a bit more consistent.

Known issues / TODOs:

Add docstrings
Tests fail with django 1.5: RegExvalidator somehow fails with message Enter a valid value. and not Enter a valid zip code.. Any pointers to fix this?

Please review!

jieter · 2015-05-17T13:56:48Z

@erikr @benkonrath ?

benkonrath · 2015-09-02T09:41:16Z

@jieter Sorry, this fell off my radar. I'll try to look at it later today.

benkonrath · 2015-09-02T18:45:59Z

localflavor/nl/validators.py

+    """
+    Validation for Dutch Sofinummers.
+
+    TODO: consider renaming to BSN


Yeah, I agree that it should be changed to BSN but let's do this as separate issue.

benkonrath · 2015-09-02T19:02:25Z

Besides those small points, it looks good. If you can fix these points, figure why the tests fail on 1.5 and get the docstrings written, then we can try to get it in the next release which is due shortly. Otherwise we'll shoot for the release after next when 1.5 support is dropped as per #170.

benkonrath · 2015-09-03T07:57:32Z

localflavor/nl/models.py

-                                                                message=self.default_error_messages['invalid'])
-        self.no_leading_zeros_regex = re.compile('[1-9]+')
+    def to_python(self, value):
+        return '%s %s' % (value[:4], value[4:])


I found a problem here. This is adding an extra space when the model field is used with the form field. For example, if I enter '1000AA', I will get the error message: 'Ensure this value has at most 7 characters (it has 8).' because the form adds a space and then this adds a space. You'll need to check if the space is already there before adding it. You should also add a test for this. Thanks.

Shouldn't we just save it in the database without space and present it with a space?

Hmm, this seems to be fixed by the last commit I pushed.

jieter · 2015-09-03T19:48:58Z

Thanks for the feedback, I'll come back to this, hopefully next week. The delay pushed it somewhat down on my todo-list...

benkonrath · 2015-09-03T21:02:17Z

Understandable. I'll keep my eyes peeled for your update. Thanks.

jieter · 2015-09-09T08:14:59Z

Just pushed some improvements. I'm not going to invest time in adding 1.5 support if it's about to be removed anyway, so I'll guess we have to wait until after the next release then?

Still working on the last bits to get this tested.

benkonrath · 2015-09-10T06:38:21Z

I think it makes sense to wait until localflavor 1.2 is released so that we can skip trying to fix the errors with Django 1.5. Although I'm not sure when that's going to happen.

The bug in the NL postal code model field in fact fixed but it would be nice to rework that bit of code to be more like the IBAN model.

You should make sure you get 100% test coverage. There's a bit of the NL postal code model field that isn't tested and the bank account model field isn't tested as well. I know that we want to deprecate and drop the EU bank account fields but we should keep the tests in until the fields are removed.

Thanks for your work on this.

jieter · 2015-09-10T08:35:26Z

Just noticed that I added forms.NLBankaccountField in this branch, so I removed it. We should not add new code to deprecate it immediately.
re-added the test for the bank account model field, you're right about that.
test coverage is now at 100%.
the Zip code form field needs the six.string_types check because apparently an array might be passed.

benkonrath · 2015-09-10T15:47:16Z

Nice catch on the forms.NLBankaccountField - I did see it but I just assumed it was already there. Nice work here - l'll merge this after localflavor 1.2 is released.

benkonrath · 2015-09-11T08:41:54Z

I forgot to mention that you should rebase this and squash all your commits into one. Thanks.

jieter · 2015-09-11T08:59:14Z

yes, will do. And what about the .. versionadded:: TODO markers?

benkonrath · 2015-09-11T09:03:06Z

We'll have to wait to see what the version will be after 1.2. You can wait to squash everything once the versionadded comments are in.

jieter · 2015-09-11T09:05:16Z

Will squash again then.

jieter · 2015-11-28T10:15:10Z

All tests pass now, should I extend the docstrings with ..versionadded::1.3 now?

benkonrath · 2015-11-29T15:24:30Z

Yeah, ..versionadded:: 1.3 would be great for any new classes.

Can you also add an entry to the changelog for this PR? The moved validator classes should be specifically mentioned as a breaking change so users know they will need to adjust their imports. I want to setup a deprecation policy for localflavor but we can sort that out before the next release.

The final (small) change it to reset the commit date. Simply running git commit --amend --reset-author on your branch should take care of it.

- Move validators to validators.py - Add model fields for existing form fields - Add form fields for existing model fields (except for NLBankAccountField) - Move tests to `tests/test_nl/` - Increase test coverage to 100% - More consistent error messages.

jieter · 2015-12-01T08:39:30Z

This should do it.

Clean up Dutch local flavor.

benkonrath · 2015-12-02T12:45:20Z

@jieter Thanks for your work on this!

jieter · 2016-04-05T09:57:46Z

@benkonrath any idea on when a version containing this PR is going to be released to pypi?

benkonrath · 2016-04-10T19:39:55Z

@jieter Within the next week or two I hope. See my comment about this in #205.

jieter · 2016-04-10T20:32:25Z

I see, thanks for the update

benkonrath mentioned this pull request Sep 2, 2015

Drop support for Django 1.5 / 1.6 and Python 2.6 #170

Closed

benkonrath reviewed Sep 2, 2015
View reviewed changes

benkonrath self-assigned this Sep 2, 2015

benkonrath reviewed Sep 3, 2015
View reviewed changes

jieter force-pushed the nl-model-fields branch 2 times, most recently from acdf6e9 to be8db15 Compare September 9, 2015 08:24

jieter force-pushed the nl-model-fields branch from 57db031 to 5a5e577 Compare September 10, 2015 08:31

jieter force-pushed the nl-model-fields branch from 5a5e577 to 338395f Compare September 11, 2015 09:03

benkonrath added this to the 1.3 milestone Nov 22, 2015

jieter force-pushed the nl-model-fields branch from 338395f to d28421d Compare November 28, 2015 09:15

jieter force-pushed the nl-model-fields branch from d28421d to 6dc8b6e Compare December 1, 2015 08:33

jieter added a commit to jieter/django-localflavor that referenced this pull request Dec 1, 2015

Update changelog.rst for changes in django#152

6f508a8

Clean up Dutch local flavor.

0d4ecfb

- Move validators to validators.py - Add model fields for existing form fields - Add form fields for existing model fields (except for NLBankAccountField) - Move tests to `tests/test_nl/` - Increase test coverage to 100% - More consistent error messages.

jieter force-pushed the nl-model-fields branch from 6f508a8 to 0d4ecfb Compare December 1, 2015 08:38

benkonrath added a commit that referenced this pull request Dec 2, 2015

Merge pull request #152 from jieter/nl-model-fields

6b37fc4

Clean up Dutch local flavor.

benkonrath merged commit 6b37fc4 into django:master Dec 2, 2015

jieter deleted the nl-model-fields branch April 5, 2016 09:55

jieter restored the nl-model-fields branch April 5, 2016 10:19

jieter mentioned this pull request Apr 5, 2016

Proposal: move repo to jazzband organisation #205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up Dutch local flavor. #152

Clean up Dutch local flavor. #152

jieter commented May 9, 2015

jieter commented May 17, 2015

benkonrath commented Sep 2, 2015

benkonrath Sep 2, 2015

benkonrath commented Sep 2, 2015

benkonrath Sep 3, 2015

jieter Sep 9, 2015

jieter Sep 9, 2015

jieter commented Sep 3, 2015

benkonrath commented Sep 3, 2015

jieter commented Sep 9, 2015

benkonrath commented Sep 10, 2015

jieter commented Sep 10, 2015

benkonrath commented Sep 10, 2015

benkonrath commented Sep 11, 2015

jieter commented Sep 11, 2015

benkonrath commented Sep 11, 2015

jieter commented Sep 11, 2015

jieter commented Nov 28, 2015

benkonrath commented Nov 29, 2015

jieter commented Dec 1, 2015

benkonrath commented Dec 2, 2015

jieter commented Apr 5, 2016

benkonrath commented Apr 10, 2016

jieter commented Apr 10, 2016

Clean up Dutch local flavor. #152

Clean up Dutch local flavor. #152

Conversation

jieter commented May 9, 2015

Known issues / TODOs:

jieter commented May 17, 2015

benkonrath commented Sep 2, 2015

benkonrath Sep 2, 2015

Choose a reason for hiding this comment

benkonrath commented Sep 2, 2015

benkonrath Sep 3, 2015

Choose a reason for hiding this comment

jieter Sep 9, 2015

Choose a reason for hiding this comment

jieter Sep 9, 2015

Choose a reason for hiding this comment

jieter commented Sep 3, 2015

benkonrath commented Sep 3, 2015

jieter commented Sep 9, 2015

benkonrath commented Sep 10, 2015

jieter commented Sep 10, 2015

benkonrath commented Sep 10, 2015

benkonrath commented Sep 11, 2015

jieter commented Sep 11, 2015

benkonrath commented Sep 11, 2015

jieter commented Sep 11, 2015

jieter commented Nov 28, 2015

benkonrath commented Nov 29, 2015

jieter commented Dec 1, 2015

benkonrath commented Dec 2, 2015

jieter commented Apr 5, 2016

benkonrath commented Apr 10, 2016

jieter commented Apr 10, 2016