Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Add support for country code filtering #107

Merged
merged 4 commits into from
Jan 21, 2016
Merged

Conversation

jlubawy
Copy link
Contributor

@jlubawy jlubawy commented Jan 13, 2016

Added support for country code filtering in geocoding forward requests, all tests pass and documentation was updated to reflect new feature.

@@ -84,6 +94,11 @@ def geojson():
return resp

@property
def country_codes(self):
"""A list of valid country codes"""
return ('ad', 'ae', 'al', 'ao', 'ar', 'at', 'au', 'ba', 'be', 'bg', 'bh', 'bn', 'br', 'bw', 'by', 'ca', 'ch', 'cl', 'cn', 'co', 'cu', 'cy', 'cz', 'de', 'dk', 'dz', 'ee', 'eg', 'es', 'fi', 'fr', 'gb', 'gf', 'gp', 'gr', 'hr', 'hu', 'id', 'ie', 'in', 'is', 'it', 'jo', 'jp', 'ke', 'kw', 'lb', 'lt', 'lu', 'lv', 'ma', 'md', 'me', 'mk', 'mt', 'mx', 'my', 'nl', 'no', 'nz', 'pe', 'ph', 'pl', 'pt', 'ro', 'rs', 'ru', 'se', 'sg', 'si', 'sk', 'sm', 'th', 'tr', 'tw', 'ua', 'us', 'uy', 've', 'vn', 'za')
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to consider other ways for defining the country codes

  • A separate text file that is read in by country_codes
  • A separate module or use the existing iso3166 module

At the very least we should reformat this tuple to be < 80 chars wide so we don't get wrapping.

@perrygeo
Copy link
Contributor

@jlubawy This looks great. Thanks for the contribution!

Aside from the note above, the only issue is the failing tests on travis which appears to be related to access_tokens and is unrelated to your commits. I'll look into it...

@perrygeo
Copy link
Contributor

Re: failing doctests on travis

  • I'll pulled the country-code branch locally and can get them passing locally with my access token
  • The problem persists after restarting the build (so it's not an intermittent error)
  • No changes were made in this branch to the .travis.yml file which might have altered the access token
  • The problem is most likely related to this access token error
  • Restarting some recent travis builds with identical travis.yml files shows that they are still passing.

still investigating...

@perrygeo
Copy link
Contributor

The mapbox access token is set up as an encrypted environment variable and

Encrypted environment variables are not available to pull requests from forks due to the security risk of exposing such information to unknown code.

@sgillies Do you know of any way around this so that doctests can pass on pull requests from forked repos? Otherwise, we can test locally and just ignore travis. It's might be a bit off-putting for contributors though.

* Make more consistent with existing code
* Use iso3166 package for country code validation
@jlubawy
Copy link
Contributor Author

jlubawy commented Jan 19, 2016

Thanks for the suggestions @perrygeo, I chose to use iso3166's country code list for validation.

One thing that's worth mentioning is that the Mapbox API doesn't support every country in this list so there may be cases where the API will return an error message such as this:

{u'message': u'Stack "zw" is not a known stack. Must be one of: cn, au, nz, sg, br, ca, at, de, es, fr, nl, gb, ie, us, ao, bw, dz, eg, ke, ma, za, ae, bh, bn, id, in, jo, kw, my, ph, th, tw, vn, ad, al, ba, be, bg, by, ch, cy, cz, dk, ee, fi, gr, hr, hu, is, it, lb, lt, lu, lv, md, me, mk, mt, no, pl, pt, ro, rs, ru, se, si, sk, sm, tr, ua, jp, ar, cl, co, cu, gf, gp, mx, pe, uy, ve'}

This error message is actually where I had gotten the original validation tuple from ('zw' is CC for Zimbabwe BTW).

My thoughts are that it's probably best to use the iso3166 package for a quick sanity check and then if the user provides a country code not supported by Mapbox they will have to handle the API error like any other they might receive. In my opinion this is better than having to maintain a separate country code list that could easily get out-of-date.

@perrygeo
Copy link
Contributor

@jlubawy I think we'll deal with travis encrypted variables issue in a different PR. This looks great, doctests pass locally, happy with using the iso3166 module for now. Let's merge it.

perrygeo added a commit that referenced this pull request Jan 21, 2016
Add support for country code filtering
@perrygeo perrygeo merged commit 82252c3 into mapbox:master Jan 21, 2016
@jlubawy jlubawy deleted the country-code branch January 22, 2016 00:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants