added name for all GB countries

benbruscella commented 8 years ago

differentiate the 3 Great Britain countries by name

therebelrobot commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

more info...

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

therebelrobot commented 8 years ago

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 commented 8 years ago

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

BrandonCopley commented 7 years ago

This has been merged with a commit.

therebelrobot / countryjs

added name for all GB countries #35