Skip to content

Added new french regions (2016 reform).#260

Merged
benkonrath merged 1 commit intodjango:masterfrom
alixmartineau:master
Nov 14, 2016
Merged

Added new french regions (2016 reform).#260
benkonrath merged 1 commit intodjango:masterfrom
alixmartineau:master

Conversation

@alixmartineau
Copy link
Copy Markdown
Contributor

@alixmartineau alixmartineau commented Nov 6, 2016

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

  • 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.

  • Add meaningful tests. 100% test coverage is not required but all
    validation edge cases should be covered.

  • Add documentation for all fields.

@benkonrath
Copy link
Copy Markdown
Member

@claudep I feel you're more qualified than I am to review this PR. Do you have time for this review? Thanks.

@claudep
Copy link
Copy Markdown
Member

claudep commented Nov 11, 2016

As a real French citizen, @aaugustin might be a better reviewer :-)

@aaugustin
Copy link
Copy Markdown
Member

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!

@benkonrath
Copy link
Copy Markdown
Member

benkonrath commented Nov 11, 2016

@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.

@alixmartineau
Copy link
Copy Markdown
Contributor Author

@benkonrath Yes, I think there is value in keeping the "old" regions for a while.

  • The French are not yet used to these regions, even though these are the official new regions.
  • There is a LOT of political controversy regarding these changes and I can see developers from certain parts of France not wanting to use this new system as a sign of protest.

@aaugustin
Copy link
Copy Markdown
Member

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 ;-)

@benkonrath
Copy link
Copy Markdown
Member

Ok, great. Thanks again for your contribution!

@benkonrath benkonrath merged commit f7c34e7 into django:master Nov 14, 2016
benkonrath added a commit that referenced this pull request Nov 14, 2016
This was referenced Nov 14, 2016
@blfpd
Copy link
Copy Markdown
Contributor

blfpd commented Nov 25, 2016

☝️ 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

@benkonrath
Copy link
Copy Markdown
Member

@batisteo I don't understand. Is there something wrong with the merged code?

@blfpd
Copy link
Copy Markdown
Contributor

blfpd commented Nov 26, 2016

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.

@benkonrath
Copy link
Copy Markdown
Member

The best thing to do is make a new pull request against master so we can discuss your proposed update. Thanks.

@blfpd
Copy link
Copy Markdown
Contributor

blfpd commented Nov 26, 2016

See my raw pull request #268

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants