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

Adds support for Enigma datasets #245

Merged
merged 1 commit into from Oct 5, 2016

Conversation

@trevorprater
Copy link

trevorprater commented Sep 28, 2016

Usage:

import pandas_datareader as pdr
df = pdr.get_data_enigma('enigma.inspections.restaurants.fl')

If ENIGMA_API_KEY does not exist in your env, it can be supplied as the second arg:

import pandas_datareader as pdr
df = pdr.get_data_enigma('enigma.inspections.restaurants.fl', 'ARIAMFHKJMISF38UT')
@femtotrader

This comment has been minimized.

Copy link
Contributor

femtotrader commented Sep 29, 2016

Thanks @trevorprater

Could you add DataReader as an entry point also ?
Ideally most DataReaders use it an entry point.

A simple test will be nice also.

StringIO can be import using

from pandas.compat import StringIO


def _request(self, url):
resp = requests.get(url)

This comment has been minimized.

Copy link
@femtotrader

femtotrader Sep 29, 2016

Contributor

You should use

resp = self.session.get(url)

so query can be cached.

@trevorprater trevorprater changed the title Adds support for enigma.io datasets Adds support for Enigma datasets Sep 29, 2016
@trevorprater

This comment has been minimized.

Copy link
Author

trevorprater commented Oct 4, 2016

Thank you for the code review, @femtotrader! I have made the suggested changes. The only thing left is for me to write some tests, I think.

@trevorprater

This comment has been minimized.

Copy link
Author

trevorprater commented Oct 5, 2016

Tests have been added and I believe all requested changes have been made. Thank you, @femtotrader!

@femtotrader

This comment has been minimized.

Copy link
Contributor

femtotrader commented Oct 5, 2016

Thanks @trevorprater

but I noticed you add directly API key yddA...Nnm into test.
That's not a very good idea because anyone looking at the code could use this API key.
That's probably not an issue for Enigma (maybe you restricted this API key) but if other users/developers want to code their own DataReader and they have a look at this code they could make a similar security error.
I suggest to use environment variable for this.
See https://docs.travis-ci.com/user/environment-variables/

Please also squash commits
http://pandas.pydata.org/pandas-docs/stable/contributing.html#contributing-your-changes-to-pandas

I suggest a commit message like "ENH: Adds support for Enigma datasets"

@trevorprater trevorprater force-pushed the trevorprater:master branch 3 times, most recently from df8ad82 to c7c496c Oct 5, 2016
@trevorprater trevorprater force-pushed the trevorprater:master branch from c7c496c to d07b904 Oct 5, 2016
@trevorprater

This comment has been minimized.

Copy link
Author

trevorprater commented Oct 5, 2016

Thank you for the additional feedback, @femtotrader.

I have added the support for an environment variable ENIGMA_API_KEY to the tests. Apologies for adding it directly to the code. I assumed it would be necessary for tests to pass. I agree that it definitely makes more sense to use an env var.

I have also squashed the commits and updated the commit message.

Trevor

@femtotrader femtotrader merged commit 061ee00 into pydata:master Oct 5, 2016
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@femtotrader

This comment has been minimized.

Copy link
Contributor

femtotrader commented Oct 5, 2016

Thanks again @trevorprater

Other contributions are welcome!

@sinhrks

This comment has been minimized.

Copy link
Member

sinhrks commented Oct 13, 2016

I think it never succeeded on Travis because we don't set ENIGMA_API_KEY. can u fix?

@jreback

This comment has been minimized.

Copy link
Contributor

jreback commented Oct 13, 2016

i just turned on the check that requires travis passing before merging

@sinhrks

This comment has been minimized.

Copy link
Member

sinhrks commented Oct 14, 2016

I've prepared #250 to fix other broken tests. For this PR, we should decide either:

  • revert this once, until the test is being ready
  • make a separate issue and leave it using SkipTest
@femtotrader

This comment has been minimized.

Copy link
Contributor

femtotrader commented Oct 16, 2016

I'm for SkipTest use.

@sinhrks

This comment has been minimized.

Copy link
Member

sinhrks commented Oct 18, 2016

How about:

  • Merge #250 and prepare v0.3.0 release.
  • If @trevorprater can update the tests by the release, it should be in 0.3.0. Otherwise, postpone the functionality to the next release.
@femtotrader

This comment has been minimized.

Copy link
Contributor

femtotrader commented Oct 18, 2016

#250 is merged.

I don't understand what you would like in tests ?
Using assertRaises like

with tm.assertRaises(ValueError):
instead of _exception
or is something else required ?

What can be the side effects of using SkipTest ?

@sinhrks

This comment has been minimized.

Copy link
Member

sinhrks commented Oct 19, 2016

What I meant is to make Enigma tests be runnable on Travis to guarantee the function. Currently no others run its tests, correct?

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.