search

sshaw / normalize_country

Convert country names and codes to a standard.
68 stars 8 forks source link

Add ability to extend list of default countries #3

Closed yuryroot closed 7 years ago

yuryroot commented 8 years ago

Hello, @sshaw!

First, thank you for fast and simple gem! Second, I want to propose my implementation of feature to extend a default countries list by user data. I think that such function will be useful.

What do you think about it?

sshaw commented 8 years ago

Hi, I think this is a nice idea but have some questions/comments:

  1. These former countries/republics have legit ISO codes, etc... so why not just add the to the existing list?
  2. One downside to adding them to the existing list is that people using to_h and to_a for say a country select field will now have what can be considered invalid countries.
  3. Why not include a YAML file with a full mapping of deprecated county codes/aliases?
yuryroot commented 8 years ago

@sshaw, there are my answers on your questions/comments:

  1. There are different types of country codes in my private project. For example: US, BR, RU, ... and WO, EO, ... In this example US, BR, RU are valid country codes, and WO, EO are patent offices.

    I think that in this case we shouldn't mix different types of countries in the default list. I think so, because it will solve only my problem and my custom data are not needed for other people by default (in particular in methods to_a and to_h, as you said in the second comment). We need a more universal solution.

  2. Yes, added countries will be in results of methods to_a and to_h. It is expected behaviour for user who extends the default list...
  3. What concrete do you mean? Do you mean that we can append a separate YAML config for deprecated countries or append mapping to the existing config?
sshaw commented 8 years ago

There are different types of country codes in my private project. For example... ... US, BR, RU are valid country codes, and WO, EO are patent offices.

Got it. Makes sense.

Do you mean that we can append a separate YAML config for deprecated countries or append mapping to the existing config?

I was thinking that all deprecated codes should be added to en.yml with a deprecated key or to a separate en.yml file. Then a method could be added say, NormalizeCountry.use_depricated!, that would add them to this list. Though I think this is out of scope for this pull request 😄

yuryroot commented 8 years ago

Thanks for your review. I will do corrections ASAP.

yuryroot commented 8 years ago

Hello, @sshaw! I'm sorry for so long absence...

I consider your comments and changed code. I also found and fixed a problem when method extend_countries incorrectly processes already existing countries. The method merges such countries now. Commit: 7fdf933bcd6494391fa9ad0c3b831b07f1d86a23.

Could you please see latest changes?

sshaw commented 8 years ago

Hi @yuryroot, thanks for the changes. Give me a day or so to have a look. 👓

sshaw commented 7 years ago

Closing due to no response. Fell free to open again if you'd like to complete.