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

CLN: Cleanup WorldBank reader #121

Merged
merged 1 commit into from Nov 25, 2015

Conversation

Projects
None yet
2 participants
@sinhrks
Member

sinhrks commented Nov 22, 2015

Closes #120 to remove urlopen.

Related #119, changed some default arg using list to None (and set them in the method).

CC @femtotrader

@davidastephens davidastephens referenced this pull request Nov 23, 2015

Closed

REL: Release 0.2.1 #123

@femtotrader

This comment has been minimized.

Contributor

femtotrader commented Nov 23, 2015

You might pass session parameter to each functions which should download data in order to also provide cache mechanism to WorldBank data reader. (CC @davidastephens )

@femtotrader

This comment has been minimized.

Contributor

femtotrader commented Nov 23, 2015

Sorry. In fact, there is now a class WorldBankReader which is a great improvement.

but you might to also pass a session parameter to download, get_countries, get_indicators, search functions and send this session parameter to WorldBankReader.__init__

@femtotrader

This comment has been minimized.

Contributor

femtotrader commented Nov 23, 2015

An other improvement might be about errors parameter.

We shouldn't use string but Enum (enum34 for Python 2) as it wil allow tab completion.

@femtotrader

This comment has been minimized.

Contributor

femtotrader commented Nov 23, 2015

A last idea (about methods of WorldBankReader)...

get_countries and get_indicators might be properties named countries and indicators as there is no parameter (except self) for these 2 WorldBankReader methods.

@femtotrader

This comment has been minimized.

Contributor

femtotrader commented Nov 23, 2015

I thought that it was the last idea... but sorry I also noticed that there is a per_page parameter in urls.

We might support chunks and use page parameter

try
http://api.worldbank.org/countries/?per_page=1000000&format=json&page=1

and

http://api.worldbank.org/countries/?per_page=100&format=json&page=1
http://api.worldbank.org/countries/?per_page=100&format=json&page=2

but this is probably out of the scope of this PR and an issue should be opened for this.

@sinhrks sinhrks force-pushed the sinhrks:wb_cln branch from 522c61c to ded7cd9 Nov 24, 2015

@sinhrks

This comment has been minimized.

Member

sinhrks commented Nov 24, 2015

@femtotrader Thanks for the comments. I've updated the PR as below. I intend this PR to cover existing functionality, and others should be handled separately.

  • Add session to WB's get_xxx funcs: Done. As other kw such as retry may be useful, passed all kwds to WorldBankReader class (same as other get_data_xxx funcs)
  • Change get_indicators and get_countries to property: Don't do. I don't think the names are very understandable for users which is different from plain get_xxx functions.
  • Change errors to enum: Don't do in this PR. Because pandas doesn't use enum. Pls prepare a PR separately.
  • Add page param: Don't do in this PR.
@femtotrader

This comment has been minimized.

Contributor

femtotrader commented Nov 24, 2015

Thanks a lot for you work. You are right! Other improvements should be done in an other PR.

@femtotrader

This comment has been minimized.

Contributor

femtotrader commented Nov 24, 2015

Sorry @sinhrks I think that there is a mistake with

return WorldBankReader().get_countries(**kwargs)

it should be

return WorldBankReader(**kwargs).get_countries()

@sinhrks sinhrks force-pushed the sinhrks:wb_cln branch from ded7cd9 to d533b60 Nov 24, 2015

@sinhrks sinhrks force-pushed the sinhrks:wb_cln branch from d533b60 to a2b8081 Nov 24, 2015

@sinhrks

This comment has been minimized.

Member

sinhrks commented Nov 24, 2015

@femtotrader Thanks! Fixed and added tests.

femtotrader added a commit that referenced this pull request Nov 25, 2015

Merge pull request #121 from sinhrks/wb_cln
CLN: Cleanup WorldBank reader

@femtotrader femtotrader merged commit ef60413 into pydata:master Nov 25, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.7%) to 95.889%
Details

@sinhrks sinhrks deleted the sinhrks:wb_cln branch Nov 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment