Skip to content

Refactor: deals with countries with the same code #55

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

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

Conversation

jimbomt
Copy link

@jimbomt jimbomt commented Feb 28, 2014

  • creates iso_countries.yml such that the key is the country's ISO code (e.g. CA)
  • updates the tests accordingly
  • introduces some helper methods
  • loads countries if not loaded (thus not requiring the developer to that by himself!)
  • introduces Canada

James Scicluna added 2 commits February 27, 2014 21:48
…some utility methods, fixes some lookups, calls .load if not loaded
@elskwid
Copy link
Collaborator

elskwid commented Apr 27, 2014

@jimbomt, thanks for the PR. I can't take it as it stands though - it changes too much at once. Having said that, I like using Carmen to get iso data and we do need to come up with a way to organized the country data outside of the country code. No doubt I will use some of what you have here and make sure to give you credit!

I'll let you know when these bits are brought in. Thanks again!

@elskwid
Copy link
Collaborator

elskwid commented Apr 27, 2014

I don't want to just shut you down. I'd like to expand on my "changes too much at once" comment. Your pull request does the following that really have nothing to do with the core issue:

  • adds personal project preferences to the .gitignore
  • changes the development ruby version
  • changes some spacing in support.rb, which was removed today by the way:)
  • changes the gem version

If you are planning on contributing to this and other projects then you should be careful not to include extraneous changes like these when adding a fix. Of particular note is changing the version of the gem, that is usually handled by the maintainers of a project. What you can do is suggest that the version be modified as a result of the changes. For instance, in this case, I have been working on v1.3.0 for a while and wouldn't likely include such a large update in the same version - I might rev to 1.4 or something.

@elskwid elskwid added this to the Two Point Oh milestone Jan 30, 2015
@jimbomt
Copy link
Author

jimbomt commented Mar 27, 2015

Hi @elskwid. Sorry for my very very late reaction to this! You are totally right about the specific project preferences and I'll make sure to clean them up.

As regards the yml/index, don't you thing that conceptually it makes more sense to have the ISO country codes as fields? Using phone area codes leads to clashes between countries (Canada and US being the obvious two). I understand it's a pretty significant change architecturally but the API is backwards-compatible (it only adds new features, rather).

How would you like to proceed with it? Or rather, is there already a version where all countries are supported? I have been too busy to keep track lately. Thanks for the feedback!

@MaicolBen
Copy link

What happen with this? I get US when call detect_country with a Canada number (even with @jimbomt fork)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants