ENH: Add ISO3 ctry codes and error arg. Fix tests, warn/exception logic #8482
State: closed

This PR adds ISO 3-digit country codes, support for World Bank Country Code exceptions, by changing the error handling and warning logic and introducing an `error` argument which toggles between ignore/warn/raise . Validation no longer filters bad country codes, prior to submitting the API request.  Instead, it attempts to catch errors in the WB response - which is a very hairy thing.  See notes added in remote_data.tst for issues.

During this PR, we attempted to clean up the JSON response, and make the module as simple as possible, without restricting the user.  In the end, it proved somewhat awkward, but the solution is a good one given the constraints of the WB API.

closes #8482 .

Updated right before merging to remove noise, and provide a summary.


Labels:
name: Enhancement
name: IO Data
description: IO issues that don't fit into a more specific label

Comments:
I think this should be much simpler. Just take the input codes and validate against the 2 or 3 code map. Prob best to simply raise if their are codes that are not valid for the specified map (and make sure its only against the 2 OR 3 map).

### Question 1

> its only against the 2 OR 3 map

Did you mean, that we should remove the 2-digit support? Wouldn't that break backwards compatibility?  Surely, this isn't what you meant. 

### Question 2

Should I create a 0.15.1.txt? Confusion? Duplicated effort?  Separate PR?

### Comment 1

b0ed056 removes the maps, and safe_cc_only, as requested. 

I'll leave it as a separate commit, until you or somebody else acknowledges the dynamism that World Bank introduces to the ISO "standard".  If you acknowledge the example of KSV, or other unknowable exceptions, and want to still stick to the more hard coded list, that's okay by me.

@jnmclarty I meant you can support both 2 and 3 digit country codes (though was sort of -1 on allowing the mixing, but on 2nd thought that is unambiguous)

will be for 0.15.1 (the file doesn't exist yet), check back this weekend

that sounds fine then

Wait, what?  What "sounds fine"?

a) leave it the old way. using json.loads + Dataframe + hacks

or 

b) using json.loads + json_normalize + hacks?

Edit: I'm indifferent.  Technically, B, is probably more robust, cause the hacks are better, but harder to read.

oh the old way is fine - least hacks as possible

if u would like to clean that part up ok! can do in another pr if u would like

So, there was one more surprise I stumbled on during testing.  In all of my previous posts, and up until and including 80dd8fd, I thought there were three kinds of country codes:
1. Standard ones
2. Unknowable Non-standard ones
3. Bad ones

Turns out, there are 4.
1. Standard ones (Eg. USA)
2. Unknowable Non-standard ones with data (Eg. KSV)
3. Unknowable Bad ones - ignored by World Bank (Eg. BLA)
4. Unknowable Bad ones - which cause API requests to fail completely (Eg. XXX)

My code, and logic, now handles all of it, IMO, extremely well and passes things appropriately to the user based on ignore/warn/raise at both the country code and indicator level. 

you'll need to squash the commits to 1-2 https://github.com/pydata/pandas/wiki/Using-Git

add a doc note in v0.15.1 (enhancements section), referencing the issue

I suspect some tests were failing and the prior author didn't figure it out - or they were do an odd data comparison that then was failing

whatever u can fix would be gr8

u can delete them (rather than comment out) as u fix

> whatever u can fix would be gr8

I already had fixed the issues with the tests.

Obviously, I rolled in things that test this ENH.

> u can delete them (rather than comment out) as u fix

Done. 7d11d12

Doc note and squash is last thing I need to do...stay tuned.

@jnmclarty looking good. just some minor changes, doc note, and squash

@jreback I commented on two now "outdated" diffs.  Do you see those?  Or are they easy to miss?

ok, pls rebase / squash and let me have a final look

Maybe it's worth a "warning" in whats new docs(?)

After all this was said and done, I'm really glad we added the 3-character ISO codes, but I'm not convinced that users won't discover issues that the previous validation method had protected against.  What I was starting with, was basically, a wb.download function which operated with only a subset of all countries.  I've given freedom to the users now, to access all countries, but for all I know, there's a 5th or 6th error case to the WB API, that will cause extremely ugly errors. 

@jnmclarty ok, this all looks good. If you would like to add a warning or note box (in the remote.rst docs) - w.r.t to some of these unretired indicators? (you can just use a code-block so it doesn't evaluate)

pls also add a note about using the 3-character codes in the docs as well (if you want to change anything else in them that is not clear, go right ahead).

maybe a note is in order (creates a box but background is white); does have to be more than a line or 2

FYI Docs updated, re-squashed. 

Did both the :ref: and ::note ...plus squashed.

You can merge now (?)

@jnmclarty you are doing gr8 actually!

I pretend that I am a user reading your comments and would be 1) no nothing, but this looks cool, 2) use this feature and care about changes. So have to appeal to both, with the 2nd being more important. You basically want to tell them hey if you are doing this, it changed, so now do this.

I can't get sphinx to build the entire docs.  Sphinx/python crashes.  I can do just remote_data.  But, sphinx still crashes right before it completes.  It builds, as below.  Maybe the crash, or the single page build, is the reason it doesn't pick up the :func: ?

![screencapture-file-c-users-jeffrey-desktop-projects-pandas34-doc-build-html-remote_data-html](https://cloud.githubusercontent.com/assets/6062071/4785473/10002f8a-5d7f-11e4-8d02-ee0adfbaa24c.png)

Squashed everything.  Only changed things identified by @jreback , and some syntax stuff related to rst.  Nothing beyond that changed since the last time I squashed.

I'm having issues with my stack/sphinx, I can't render the entire docs.

If somebody ( @jorisvandenbossche ?) could render the entire docs, and take a look at whatsnew and remote_data, as well as the docstring to `io.wb.download()` prior to this being merged, that would be helpful.  Or, even a detail-oriented rst-pro would be able to take one glance at the diffs and OK it.  Either way, I'll review the output rendered by the server.

Formatting of the docs is looking OK, apart from the one comment I gave. 

But another thing: you added a note in the API changes section, but normally we shouldn't do behaviour changing API changes for 0.15.1. 
But the api changes note is not fully clear to me: does it just now raise warnings/errors where previously happened nothing (empty result), or will you really get a different returned result in some cases? 

this is one of those
adding arguments cases 

so it's an enhancement (but I usually put these in api change section)

I tried to improve clarity in whatsnew, and fixed the single/double quote thing, then re-squashed.

OK on the API change, it sounds indeed more like a bug fix/enhancement.

@jorisvandenbossche yep, should be in the Enhancements section (we put adding args stuff their right?)

@jnmclarty can you move to Enhancements then

Confused, I had split it, and put a note in each.  (ie, I had a note for the errors in API, and functional change in Enhancements)

...but, I did what you asked.