Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added full names and numerical codes for Swedish counties #117

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dessibelle
Copy link

I have added the following alternative choice options to se_counties.py:

NUMERICAL_COUNTY_CODE_CHOICES contains the numerical two-digit couty codes which has more or less replaced the alphabetical codes nowadays. Also, these numerical codes is always the leading digits in the codes for municipailites and smaller subdivisions, which can be beneficial.

FULL_COUNTY_NAME_CHOICES contains the full name of each county. The current labels given in SE_COUNTIES is good when it is obvioius to the user that he/she is selecting a county, but they are not identical to the full county name. Also, in Swedish the county names differ somewhat, e.g. "Skåne län" as opposed to the more common "Stockholms län" (ending with genitive case s), which the Swedish i18n should honor.

For backwards compatibility I have chosen to keep both the alphabetical codes and the shortened names (lacking the "county"-part) in the original SE_COUNTIES list, although I guess one could argue numerical codes and full names would be a better option.

I also added a SECountyField field that contributes a few accessor methods to the model class, allowing the you to access numerical county codes and full county names for a given field like so:

from django.db import models
from localflavor.se.models import SECountyField
from .se_counties import COUNTY_CHOICES, NUMERICAL_COUNTY_CODE_CHOICES, FULL_COUNTY_NAME_CHOICES

class MyModel(models.Model):
    county = SECountyField()

obj = MyModel(county='AB')

obj.get_county_numerical_code() # '01'
obj.get_county_full_name()      # 'Stockholm County'
obj.get_county_short_name()     # 'Stockholm'
obj.get_county_display()        # 'Stockholm'
obj.county                      # 'AB'

If I have understood the other pull requests correctly, localizations is managed through Transifex. I'd be glad to add Swedish localizations for the introduced changes if I could get access.

I have added the following dictionaries and functions to se_counties:

NUMERICAL_COUNTY_CODES contains the numerical two-digit couty codes
which has more or less replaced the alphabetical codes nowadays. Also,
these numerical codes is always the leading digits in the codes for
municipailites and smaller subdivisions, which can be beneficial.

FULL_COUNTY_NAMES contains the full names of each county. The current
labels given in SE_COUNTIES is good when it is obvioius to the user
that he/she is selecting a county, but they are not identical to the
full county name. Also, in Swedish the county names differ somewhat,
e.g. "Skåne län" as opposed to the more generic "Stockholms län"
(ending with genitive case s), which the Swedish i18n should honor.

numerical_county_code() and full_county_name() are simply accessor
methods for the above dictionaries, returning the corresponding values
given a alphabetical county code.

For backwards compatibility I have chosen to keep both the
alphabetical codes and the shortened names (lacking the "county"-part)
in the original SE_COUNTIES list, although I guess one could argue
numerical codes and full names would be a better option.
The decorator adds get_FIELDNAME_numerical_code() and
get_FIELDNAME_full_name() methods to django models for all FIELDNAME
values supplied in the field_names tuple. Ideally this can be used on
all model fields that uses COUNTY_CHOICES for the choices argument.

@county_decorator(field_names('field',))
class MyClass:
    field = models.CharField(choices=se_counties=COUNTY_CHOICES)
@jezdez jezdez modified the milestone: 1.2 Nov 16, 2014
}


def numerical_county_code(county_code):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of those helpers won't be necessary as they just wrap dict functionality, which seems moot. Please remove.

@jezdez
Copy link
Contributor

jezdez commented Nov 21, 2014

I think the decorator is a nifty feature but frankly out of scope of localflavor. Instead I'd much rather see this implemented as a model field class that uses the contribute_to_class/pre_save hooks, like here: https://github.com/carljm/django-model-utils/blob/b54d4652a3c90fe622c504b4f42260120d2b6f9d/model_utils/fields.py#L200-L235

@jezdez
Copy link
Contributor

jezdez commented Nov 21, 2014

Ah, regarding Transifex please request access to the team on the platform. We leave the management of language teams to the appropriate language coordinators.

@dessibelle
Copy link
Author

I understand, I'll remove the helper functions and the decorator. Would you say a field based implementation using contribute_to_class / pre_save as per the example you posted is in scope of localflavor? I'd be happy to refactor if that's the case.

@jezdez
Copy link
Contributor

jezdez commented Nov 21, 2014

@dessibelle Yeah, a field would work as we ship with plenty of them. I just want to keep the API we publish to the users as common as possible.

@dessibelle
Copy link
Author

@jezdez Sure, I totally understand, and I value your clarifications. I had basically just solved the problem for myself, without too much knowledge of the API in a broader sense.

…sors for full county name and numerical codes.

Also renamed `FULL_COUNTY_NAMES` and `NUMERICAL_COUNTY_CODES` in `se_counties.py` into `NUMERICAL_COUNTY_CODE_CHOICES` and `FULL_COUNTY_NAME_CHOICES` respectively, as well as converted them both to tuple lists rather than dicts in order for them to be supplied as the `chocies` argument for the field.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants