Skip to content

CLN: avoid catching Exception in _choose_path #28205

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

Merged
merged 4 commits into from
Sep 2, 2019

Conversation

jbrockmendel
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@WillAyd
Copy link
Member

WillAyd commented Aug 28, 2019

If you are deep diving on this any chance you can add a docstring for func to help future readers?

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

minor nit but otherwise lgtm - I think this certainly reads nicer

if not isinstance(res_fast, DataFrame):
return path, res

if not res_fast.columns.equals(group.columns):
Copy link
Member

Choose a reason for hiding this comment

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

Can you add this condition to the check above?

Copy link
Member Author

Choose a reason for hiding this comment

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

for coverage purposes Id rather leave them separate. i.e. i want to be able to see that both cases are reached

@WillAyd WillAyd added this to the 1.0 milestone Aug 30, 2019
@jreback
Copy link
Contributor

jreback commented Sep 2, 2019

lgtm. @WillAyd ?

@WillAyd WillAyd merged commit 562f423 into pandas-dev:master Sep 2, 2019
@WillAyd
Copy link
Member

WillAyd commented Sep 2, 2019

Thanks @jbrockmendel

@jbrockmendel jbrockmendel deleted the choose_path branch September 2, 2019 22:54
MarcoGorelli pushed a commit to MarcoGorelli/pandas that referenced this pull request Sep 3, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants