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

Skip tests on network error #3893

Merged
merged 2 commits into from
Jun 19, 2013
Merged

Skip tests on network error #3893

merged 2 commits into from
Jun 19, 2013

Conversation

gliptak
Copy link
Contributor

@gliptak gliptak commented Jun 13, 2013

No description provided.

# sanity checking
t= np.array(pan)
assert np.issubdtype(t.dtype, np.floating)
except IOError:
Copy link
Member

Choose a reason for hiding this comment

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

it might be best if use a try:... suite whereever you perform io (and nowhere else) rather than just wrapping everything into a huge try: clause. it's going to be a huge pain to track down which of these is throwing the io error down the line. as a rule try (no pun intended) keeping try: clauses as small as possible so that disambiguating errors is easier. also no need to put any of the post io code in the try:... suite since if you're there there wasn't a raise

@jreback
Copy link
Contributor

jreback commented Jun 18, 2013

closing in favor of #3914

@jreback jreback closed this Jun 18, 2013
@gliptak
Copy link
Contributor Author

gliptak commented Jun 18, 2013

@jreback while I understand that the decorator is a better approach, this pull request also split up tests into smaller functions, which make identifying errors easier ... I think this is a small value add regardless.

@jreback jreback reopened this Jun 18, 2013
@jreback
Copy link
Contributor

jreback commented Jun 18, 2013

@gliptak np.....

cc @jtratner

shall we merge this first? then #3914 which might need a rebase after?

@jtratner
Copy link
Contributor

I'm going to be home and able to check in about 30mins, do you mind waiting
so I can check? I think my PR covers nearly all of this already (including
switching out imports etc)
On Jun 18, 2013 7:38 PM, "jreback" notifications@github.com wrote:

@gliptak https://github.com/gliptak np.....

cc @jtratner https://github.com/jtratner

shall we merge this first? then #3914https://github.com/pydata/pandas/issues/3914which might need a rebase after?


Reply to this email directly or view it on GitHubhttps://github.com//pull/3893#issuecomment-19650825
.

@jtratner
Copy link
Contributor

In particular - the network and with connectivity decorators make it
unnecessary to catch IOErrors in test.
On Jun 18, 2013 7:40 PM, "Jeffrey Tratner" jeffrey.tratner@gmail.com
wrote:

I'm going to be home and able to check in about 30mins, do you mind
waiting so I can check? I think my PR covers nearly all of this already
(including switching out imports etc)
On Jun 18, 2013 7:38 PM, "jreback" notifications@github.com wrote:

@gliptak https://github.com/gliptak np.....

cc @jtratner https://github.com/jtratner

shall we merge this first? then #3914https://github.com/pydata/pandas/issues/3914which might need a rebase after?


Reply to this email directly or view it on GitHubhttps://github.com//pull/3893#issuecomment-19650825
.

@jreback
Copy link
Contributor

jreback commented Jun 18, 2013

also pls look at #3916, prob related

@jtratner
Copy link
Contributor

Uh but you shouldn't merge my other PR. Have to make one edit like I said
earlier.
On Jun 18, 2013 7:41 PM, "Jeffrey Tratner" jtratner@gmail.com wrote:

In particular - the network and with connectivity decorators make it
unnecessary to catch IOErrors in test.
On Jun 18, 2013 7:40 PM, "Jeffrey Tratner" jeffrey.tratner@gmail.com
wrote:

I'm going to be home and able to check in about 30mins, do you mind
waiting so I can check? I think my PR covers nearly all of this already
(including switching out imports etc)
On Jun 18, 2013 7:38 PM, "jreback" notifications@github.com wrote:

@gliptak https://github.com/gliptak np.....

cc @jtratner https://github.com/jtratner

shall we merge this first? then #3914https://github.com/pydata/pandas/issues/3914which might need a rebase after?


Reply to this email directly or view it on GitHubhttps://github.com//pull/3893#issuecomment-19650825
.

@jreback
Copy link
Contributor

jreback commented Jun 18, 2013

no merging until positive confirmation from both of you (kind of like it takes 2 keys to blow up the world)

@gliptak
Copy link
Contributor Author

gliptak commented Jun 19, 2013

@jreback Maybe you can merge this pull request (as it "compiles" alone), than retrofit #3914 to HEAD? There are also @network functions in #3822.

@jtratner
Copy link
Contributor

The problem is that all of the try: catch URLErrror...check google.com idiom is handled transparently by the new network and with_connectivity_check decorators. So if we merge the other commit first, you can replace all of the try/catch statements with a:

@with_connectivity_check("http://www.google.com")

at the top.

@gliptak
Copy link
Contributor Author

gliptak commented Jun 19, 2013

@jtratner Jeff, do you see this pull request working as is (without implementing your improvements)?

@jtratner
Copy link
Contributor

@gliptak one of us will have to edit a pull request to make the final merge happen, haha. The only thing is that you have more domain experience with this module than I do, so it would be great if you could decide which tests should check connectivity ahead of time (@with_check_connectivity(check_before_test=True)/afterwards(@with_check_connectivity) and which tests should just ignore connectivity errors altogether (@network)). I tried to do this, but it was rather arbitrary.

@jtratner
Copy link
Contributor

@jreback why don't you just merge this first and then I'll edit the other one.

@jreback jreback merged commit 6cedd92 into pandas-dev:master Jun 19, 2013
@jreback
Copy link
Contributor

jreback commented Jun 19, 2013

@gliptak thank you

@gliptak
Copy link
Contributor Author

gliptak commented Jun 19, 2013

@jreback no problem
@jtratner please let me know if you are expecting changes from me

@jtratner
Copy link
Contributor

It's all good, ended up being a breeze to merge.
On Jun 19, 2013 7:17 PM, "gliptak" notifications@github.com wrote:

@jreback https://github.com/jreback no problem
@jtratner https://github.com/jtratner please let me know if you are
expecting changes from me


Reply to this email directly or view it on GitHubhttps://github.com//pull/3893#issuecomment-19721651
.

@gliptak gliptak deleted the googlenetwork branch May 10, 2016 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants