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

CLN: catch less in pd.io #28862

Closed
wants to merge 1 commit into from

Conversation

jbrockmendel
Copy link
Member

@bashtage (only tangentially related to this PR) there is an except Exception in pandas.io.stata that I'd like to make more specific. Any suggestions?

@bashtage
Copy link
Contributor

bashtage commented Oct 9, 2019

This is an intentional catch all that ensures the file gets cleanly closed. It could legitimately encounter a wide range of errors include memory exceptions, io exceptions, windows exceptions and probably others. The only reason why Exception is caught is so it can re reraised.

The Exception isn't actually needed since a bare raise will raise the origin exception, although if you remove it you will then have an except with no specified exception, which some don't like.

@jreback jreback added the Clean label Oct 11, 2019
else:
return tools.to_datetime(parsed, errors="ignore")

else:
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like a new path, is it actually hit?

this code seems to have gotten more complex as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

The to_datetime call below corresponds to the to_datetime call on 3274 in master. This is just moving it outside of the try/except.

@jreback jreback added this to the 1.0 milestone Nov 2, 2019
return generic_parser(date_parser, *date_cols)
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this or the bottom else at all?

IOW if you just remove these and have the last line of the function do what you do on 3293 right now it seems cleaner?

Copy link
Member Author

Choose a reason for hiding this comment

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

this isnt obvious, largely because we are dealing with a user-provided date_parser function. ill take another look

@jbrockmendel
Copy link
Member Author

Closing to clear the queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants