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 name for all GB countries #35

Closed
wants to merge 3 commits into from
Closed

added name for all GB countries #35

wants to merge 3 commits into from

Conversation

benbruscella
Copy link
Contributor

differentiate the 3 Great Britain countries by name

@benbruscella benbruscella changed the title added name for other 2 GB countries Scotland and Wales added name for all GB countries Feb 25, 2016
@therebelrobot
Copy link
Owner

looks great! For additions like this, I generally prefer to have tests added that specifically test what you're adding/fixing. In this case, adding mocha tests to see results if pulled by ISO, the fail/success cases for referencing by name.

If you could add those in, I can see about getting it merged in.

@benbruscella
Copy link
Contributor Author

Thanks for reminding me to test! done.

I think GB raises some interesting questions for your API though, specifically, what data should be returned for ISO queries with multiple countries?

Example:

> cj.info("GB", "ISO2")
{ name: 'Scotland',
  flag: '',
  geoJSON: {},
  ISO: { '2': 'GB', '3': 'GBR', alpha2: 'GB', alpha3: 'GBR' },
  provinces:
   [ 'Aberdeen City',
     'Aberdeenshire',
     'Angus',
     'Argyll and Bute',
     'City of Edinburgh',
     'Clackmannanshire',
     'Dumfries and Galloway',
     'Dundee City',
     'East Ayrshire',
     'East Dunbartonshire',
     'East Lothian',
     'East Renfrewshire',
     'Eilean Siar (Western Isles)',
     'Falkirk',
     'Fife',
     'Glasgow City',
     'Highland',
     'Inverclyde',
     'Midlothian',
     'Moray',
     'North Ayrshire',
     'North Lanarkshire',
     'Orkney Islands',
     'Perth and Kinross',
     'Renfrewshire',
     'Shetland Islands',
     'South Ayrshire',
     'South Lanarkshire',
     'Stirling',
     'The Scottish Borders',
     'West Dunbartonshire',
     'West Lothian' ],
  tld: [],
  wiki: 'http://en.wikipedia.org/wiki/scotland' }

I think this really should be returning an array?

I'd propose that info() always return an array of countries, with the country name as the key. It's a breaking change though, which is a shame for a special case.

What are your thoughts on this?

@benbruscella
Copy link
Contributor Author

more info...

https://en.wikipedia.org/wiki/ISO_3166-2:GB

@therebelrobot
Copy link
Owner

I'd probably end up opting for a simple array of results, since name is specific to one language alone, and I don't want to be making assumptions about the preferred language of the user.

It would be a breaking change :/ I don't think there's any way around that though, the current return is inconsistent.

@therebelrobot
Copy link
Owner

I guess I know what I'm working on this weekend :P

@BrandonCopley
Copy link
Collaborator

This has been merged with a commit.

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.

3 participants