Closed alixmartineau closed 7 years ago
@claudep I feel you're more qualified than I am to review this PR. Do you have time for this review? Thanks.
As a real French citizen, @aaugustin might be a better reviewer :-)
Indeed France messed up with regions once again and this patch correctly reflects the latest version of the mess. The data is all right.
@claudep This looks ready for checkin to me.
@alix- Thanks!
@alix- Thanks for the PR. We're trying to keep only current region names in localflavor rather than keeping historical data as well. Here's some information the policy: https://django-localflavor.readthedocs.io/en/latest/#backwards-compatibility We'll be switching to the django warnings system shortly so that we can deprecate fields and provide help with migrating data (there are PRs open for this).
For this case, do you think there's a use case for keeping the existing data at least for a little while? If so, then this PR seems like a good option. At some point we could depreciate the original field and remove it. I'm curious to hear people's thoughts on this. Thanks.
@benkonrath Yes, I think there is value in keeping the "old" regions for a while.
When I read the patch I wrote "yes keeping the old definition for backwards compatibility is smart". Then I noticed it was against the guidelines so I removed that comment. FYI ;-)
Ok, great. Thanks again for your contribution!
:point_up: Wait, the names changed by June/July, official in November.
Occitanie, Hauts-de-France, Nouvelle-Aquitaine…
fr.wikipedia.org/wiki/Région_française#Liste_des_régions_actuelles
@batisteo I don't understand. Is there something wrong with the merged code?
One example: I see Languedoc-Roussillon-Midi-Pyrénées instead of Occitanie. The new regions are one year old, but the names wasn't fixed until this summer.
And it seems like the INSEE just "refactored" their site so the URL is not correct anymore.
The best thing to do is make a new pull request against master so we can discuss your proposed update. Thanks.
See my raw pull request #268
Added new french regions dataset (http://www.insee.fr/fr/methodes/nomenclatures/cog/).
We should keep the old regions and refer to these ones as "new regions", as the government's decision to change french regions is quite controversial. Therefore, it's a good idea to leave the option to django-localflavour users to keep using the old regions system, at least for a little while.
All Changes
[x] Add an entry to the docs/changelog.rst describing the change.
[x] Add an entry for your name in the docs/authors.rst file if it's not already there.
[x] Adjust your imports to a standard form by running this command:
isort --recursive --line-width 120 localflavor tests
New Fields Only
[x] Prefix the country code to all fields.
[x] 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.
[x] Add meaningful tests. 100% test coverage is not required but all validation edge cases should be covered.
[x] Add documentation for all fields.