BUG: changed io.wb.get_countries URL #6008

clarkfitzg · 2014-01-20T06:04:15Z

Only 50 countries show up for the get_countries() call in io.wb, rather than the 256 that actually exist. Looks like a typo in the World Bank Documentation. The workaround is to request 1000 countries. Running this change by @vincentarelbundock.

jreback · 2014-01-21T03:06:37Z

can u add a test which can validate this ?

clarkfitzg · 2014-01-21T14:37:08Z

Sure, I'll add a test to check if 'Zimbabwe' is the last country in the data frame. I'm brand new to this though, so I could use some help with writing the test properly.

TomAugspurger · 2014-01-21T14:40:23Z

Just push your changes to this branch and then we'll take a look. There's a few notes here that should help you get started, specifically the section on tests that need network connectivity.

jreback · 2014-01-22T15:47:10Z

pandas/io/tests/test_wb.py

+    @slow
+    @network
+    def test_wdi_get_countries(self):
+        raise nose.SkipTest


you are skipping the test?

jreback · 2014-01-22T15:47:51Z

is your master update to date?

git fetch upstream/master
git rebase -i upstream/master

clarkfitzg · 2014-01-23T04:17:17Z

Master is updated. I copied from the other tests in the test_wb.py - they all had raise nose.SkipTest as the first line. Looks like they all fail now when I run nosetests pandas:

AssertionError: u'GDP per capita (constant 2005 US$)' != u'GDP per capita (constant 2000 US$)'
-------------------- >> begin captured stdout << ---------------------
Failed: AssertionError("u'GDP per capita (constant 2005 US$)' != u'GDP per capita (constant 2000 US$)'",)

--------------------- >> end captured stdout << ----------------------

clarkfitzg · 2014-01-23T04:20:42Z

I'll take a look at some other test modules and see if I can rewrite these.

jreback · 2014-01-23T11:05:52Z

just delete the raise nose.SkipTest lines entirely

jreback · 2014-01-23T15:18:04Z

can you also add a release note in 0.13.1 (reference this PR), about this bugfix

jreback · 2014-01-24T22:27:24Z

can you rebase, add release notes and remove those SkipTest lines?

thanks

clarkfitzg · 2014-01-24T23:38:46Z

Yes. Should I do this before the tests are passing though?

jreback · 2014-01-24T23:41:05Z

yes always should write the first to make sure that they in fact fail (otherwise could have a bogus test)

clarkfitzg · 2014-01-25T00:05:42Z

Makes sense! I'll make the change tonight.

BLD/DOC: report context on warnings and add :okwarning: to ipython_directive

ghost · 2014-01-25T17:27:59Z

This rebase catastrophe is getting out of hand. I don't think we should require
contributors to have that much git fu. It's unpleasent to them and a constant nuisance to us.

cpcloud · 2014-01-25T18:09:53Z

@y-p Should we just take care of it ourselves? I distinctly remember admonishment from you about learning git ... not that it wasn't completely warranted, but I think it's important to learn about rebase if you're going to contribute to OSS using git. That said, for a DBC it's probably a little much

ghost · 2014-01-25T18:36:18Z

Your PRs showed that you were substantially invested in writing code and you made
a truely collosal mess of your PR history at the time, so it seemed right to push you
towards learning the tools. At this point, you probably know git better then me.

If you'll recall, I did not teach you git (except on specific points you asked about).
What I suggested is that if you're not comfortable with git that you leave it to the
maintainers to sort out, which I did. It really is less work often, and less stressful
on well-intentioned contributors who are otherwise made to run the git gauntlet.

The amount of impromptu git tutorials that span more comments then the actual discussion
on the substance of the PR is really high here. Not every typo fix PR mandates that the submitter
have an in-depth understanding of the git merge-model. Leave'em alone.

If someone really screws up, admonish. if someone starts submitting lots of good PRs
marred only by bad git fu, by all means walk them through it and reciprocate their efforts.
Otherwise, pull-fix-push-close (while maintaining authorship). I've often done that in the past.

You do this because you're truely nice, generous people who like to share knowledge
and help out others. But sometimes, it's just too much.

And I sincerly hope you've learned to shrug off my admonishments. It's just a thing.

cpcloud · 2014-01-25T18:37:35Z

Fair enough.

ghost · 2014-01-25T18:41:55Z

btw, the rebase+force push is kind of unique to pandas. Many (most?) projects are actually -1 on
rebase+force pushing, as is GH itself.

cpcloud · 2014-01-25T18:43:45Z

Good to know. I'm not sure I agree with no rebases ATB ... but that's a discussion for another point in the space time continuum.

clarkfitzg · 2014-01-25T19:15:29Z

My 2 cents- I found a tiny bug that was trivial to correct. Just wanted to
point it out and give back a bit to the software I use. Probably would have
been better to do this in a bug report as my knowledge of Git and testing
is minimal.

If you could point me to a resource to learn more about Git and Testing
from the ground up I would appreciate it.

Thanks,
Clark

On Saturday, January 25, 2014, Phillip Cloud [email protected]
wrote:

Good to know. I'm not sure I agree with no rebases ATB ... but that's a
discussion for another point in the space time continuum.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6008#issuecomment-33296370
.

* clarkfitzg-master: changed get_countries URL Added test for wb.get_countries

ghost · 2014-01-25T19:27:51Z

@clarkfitzg , thanks for the report and the fix. We appreciate both and just to make it clear:
that whole rant was really not about you or this PR :)

I've neatened your PR and pushed to master as 70b1ad4. It'll be part of the soon-to-be-released
0.13.1.

Re git tutorials, I've found http://pcottle.github.io/learnGitBranching/ to be a useful resource for
git newcomers. You should remember that absolutely everyone finds git difficult at some point
but most find it indispensable once over the hump.

ghost · 2014-01-25T19:37:58Z

The pandas GH wiki also has some git sections: https://github.com/pydata/pandas/wiki

cpcloud · 2014-01-25T20:45:57Z

@y-p That's a nice tutorial. Very co-worker friendly :)

jreback · 2014-01-25T20:51:46Z

@cpcloud maybe add a link in the pandas github page to that one?

cpcloud · 2014-01-25T20:59:49Z

done

clarkfitzg · 2014-01-26T03:36:14Z

@y-p, @cpcloud, and @jreback, I very much appreciate the follow up and the hand holding. Must be a tremendous amount of effort on your parts to organize all these different contributions.

This was my first attempt at contributing to OSS, and now I have a better idea of how to go about it. Next time I'll bring more git-fu to the game.

cpcloud · 2014-01-26T03:46:07Z

Np happy to help

On Saturday, January 25, 2014, Clark Fitzgerald [email protected]
wrote:

@y-p https://github.com/y-p, @cpcloud https://github.com/cpcloud, and
@jreback https://github.com/jreback, I very much appreciate the follow
up and the hand holding. Must be a tremendous amount of effort on your
parts to organize all these different contributions.

This was my first attempt at contributing to OSS, and now I have a better
idea of how to go about it. Next time I'll bring more git-fu to the game.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6008#issuecomment-33308253
.

Best,
Phillip Cloud

changed get_countries URL

2847c30

Added test for wb.get_countries

ae23932

jreback reviewed Jan 22, 2014
View reviewed changes

pandas/io/tests/test_wb.py

@slow

@network

def test_wdi_get_countries(self):

raise nose.SkipTest

Copy link

Contributor

jreback Jan 22, 2014

Choose a reason for hiding this comment

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

you are skipping the test?

commented out nose.SkipTest

ed3e358

y-p and others added 7 commits January 24, 2014 17:48

Merge pull request #6082 from y-p/PR_ipython_directive

fe8fe38

BLD/DOC: report context on warnings and add :okwarning: to ipython_directive

DOC/BLD: ipython_directive cleanups

e60ddfe

changed get_countries URL

622cd88

Added test for wb.get_countries

20beca1

commented out nose.SkipTest

e298c6f

documenting

1d1427a

merging

1b7d291

ghost pushed a commit that referenced this pull request Jan 25, 2014

Merge pull request #6008 from clarkfitzg/master (reworked)

70b1ad4

* clarkfitzg-master: changed get_countries URL Added test for wb.get_countries

ghost closed this Jan 25, 2014

clarkfitzg mentioned this pull request Mar 11, 2014

API: default value for read_excel sheet #6576

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: changed io.wb.get_countries URL #6008

BUG: changed io.wb.get_countries URL #6008

clarkfitzg commented Jan 20, 2014

jreback commented Jan 21, 2014

clarkfitzg commented Jan 21, 2014

TomAugspurger commented Jan 21, 2014

jreback Jan 22, 2014

jreback commented Jan 22, 2014

clarkfitzg commented Jan 23, 2014

clarkfitzg commented Jan 23, 2014

jreback commented Jan 23, 2014

jreback commented Jan 23, 2014

jreback commented Jan 24, 2014

clarkfitzg commented Jan 24, 2014

jreback commented Jan 24, 2014

clarkfitzg commented Jan 25, 2014

ghost commented Jan 25, 2014

cpcloud commented Jan 25, 2014

ghost commented Jan 25, 2014

cpcloud commented Jan 25, 2014

ghost commented Jan 25, 2014

cpcloud commented Jan 25, 2014

clarkfitzg commented Jan 25, 2014

ghost commented Jan 25, 2014

ghost commented Jan 25, 2014

cpcloud commented Jan 25, 2014

jreback commented Jan 25, 2014

cpcloud commented Jan 25, 2014

clarkfitzg commented Jan 26, 2014

cpcloud commented Jan 26, 2014

BUG: changed io.wb.get_countries URL #6008

BUG: changed io.wb.get_countries URL #6008

Conversation

clarkfitzg commented Jan 20, 2014

jreback commented Jan 21, 2014

clarkfitzg commented Jan 21, 2014

TomAugspurger commented Jan 21, 2014

jreback Jan 22, 2014

Choose a reason for hiding this comment

jreback commented Jan 22, 2014

clarkfitzg commented Jan 23, 2014

clarkfitzg commented Jan 23, 2014

jreback commented Jan 23, 2014

jreback commented Jan 23, 2014

jreback commented Jan 24, 2014

clarkfitzg commented Jan 24, 2014

jreback commented Jan 24, 2014

clarkfitzg commented Jan 25, 2014

ghost commented Jan 25, 2014

cpcloud commented Jan 25, 2014

ghost commented Jan 25, 2014

cpcloud commented Jan 25, 2014

ghost commented Jan 25, 2014

cpcloud commented Jan 25, 2014

clarkfitzg commented Jan 25, 2014

ghost commented Jan 25, 2014

ghost commented Jan 25, 2014

cpcloud commented Jan 25, 2014

jreback commented Jan 25, 2014

cpcloud commented Jan 25, 2014

clarkfitzg commented Jan 26, 2014

cpcloud commented Jan 26, 2014