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

BUG: changed io.wb.get_countries URL #6008

Closed
wants to merge 10 commits into from

Conversation

@clarkfitzg
Copy link
Contributor

commented Jan 20, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2014

can u add a test which can validate this ?

@clarkfitzg

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2014

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.

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

This comment has been minimized.

Copy link
@jreback

jreback Jan 22, 2014

Contributor

you are skipping the test?

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2014

is your master update to date?

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2014

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2014

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

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2014

just delete the raise nose.SkipTest lines entirely

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2014

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

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2014

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

thanks

@clarkfitzg

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2014

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

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2014

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

@clarkfitzg

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2014

Makes sense! I'll make the change tonight.

y-p and others added 7 commits Jan 25, 2014
y-p
Merge pull request #6082 from y-p/PR_ipython_directive
BLD/DOC: report context on warnings and add :okwarning: to ipython_directive
y-p
@ghost

This comment has been minimized.

Copy link

commented Jan 25, 2014

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

This comment has been minimized.

Copy link
Member

commented Jan 25, 2014

@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

This comment has been minimized.

Copy link

commented Jan 25, 2014

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

This comment has been minimized.

Copy link
Member

commented Jan 25, 2014

Fair enough.

@ghost

This comment has been minimized.

Copy link

commented Jan 25, 2014

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

This comment has been minimized.

Copy link
Member

commented Jan 25, 2014

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2014

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 notifications@github.com
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
.

ghost pushed a commit that referenced this pull request Jan 25, 2014
y-p
Merge pull request #6008 from clarkfitzg/master (reworked)
* clarkfitzg-master:
  changed get_countries URL
  Added test for wb.get_countries
@ghost

This comment has been minimized.

Copy link

commented Jan 25, 2014

@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 ghost closed this Jan 25, 2014

@ghost

This comment has been minimized.

Copy link

commented Jan 25, 2014

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

@cpcloud

This comment has been minimized.

Copy link
Member

commented Jan 25, 2014

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

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2014

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

@cpcloud

This comment has been minimized.

Copy link
Member

commented Jan 25, 2014

done

@clarkfitzg

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2014

@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

This comment has been minimized.

Copy link
Member

commented Jan 26, 2014

Np happy to help

On Saturday, January 25, 2014, Clark Fitzgerald notifications@github.com
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

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.