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

BUG: read_csv: fix wrong exception on permissions issue #32737

Conversation

roberthdevries
Copy link
Contributor

@roberthdevries roberthdevries commented Mar 15, 2020

Get rid of all error printf's and produce proper Python exceptions

@roberthdevries
Copy link
Contributor Author

Will add tests and whatsnew later, for now curious if it passes the windows build.

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.

cool thanks. Would be nice to get green

@@ -62,6 +66,11 @@ void *new_file_source(char *fname, size_t buffer_size) {
#endif
if (fs->fd == -1) {
free(fs);
#ifdef USE_WIN_UTF16
Copy link
Member

Choose a reason for hiding this comment

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

Is this not applicable to the above calls? If it is you can just make a macro for it in portable.h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the special case handling for Windows. I checked the python code and PyErr_SetFromErrnoWithFilename works also in Windows

@WillAyd WillAyd added the Error Reporting Incorrect or improved errors from pandas label Mar 16, 2020
@roberthdevries roberthdevries force-pushed the fix-23784-fix-wrong-exception-on-permissions branch 2 times, most recently from a9393e9 to ffcc4f6 Compare March 17, 2020 21:24
@roberthdevries roberthdevries changed the title WIP: read_csv: fix wrong exception on permissions issue read_csv: fix wrong exception on permissions issue Mar 17, 2020
os.chmod(path, 0) # make file unreadable
with pytest.raises(PermissionError, match=msg) as e:
parser.read_csv(path)
assert path == e.value.filename
Copy link
Member

Choose a reason for hiding this comment

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

i think this assertion needs to be un-indented

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are absolutely right and the test before this one had the same issue (I copied it from that test). That one is fixed as well.

@roberthdevries roberthdevries force-pushed the fix-23784-fix-wrong-exception-on-permissions branch from 3f29d8d to f14b501 Compare March 18, 2020 19:55
@roberthdevries roberthdevries changed the title read_csv: fix wrong exception on permissions issue BUG: read_csv: fix wrong exception on permissions issue Mar 18, 2020
@jreback jreback added this to the 1.1 milestone Mar 19, 2020
@jreback jreback added the IO CSV read_csv, to_csv label Mar 19, 2020
@jreback jreback requested a review from gfyoung March 19, 2020 00:08
@jreback
Copy link
Contributor

jreback commented Mar 19, 2020

@gfyoung if any comments.

Co-Authored-By: gfyoung <gfyoung17+GitHub@gmail.com>
@jreback jreback merged commit dec5211 into pandas-dev:master Mar 19, 2020
@roberthdevries roberthdevries deleted the fix-23784-fix-wrong-exception-on-permissions branch March 19, 2020 19:07
SeeminSyed pushed a commit to CSCD01-team01/pandas that referenced this pull request Mar 22, 2020
…2737)

* Generate exception from the C code in the proper manner

Get rid of all error printf's and produce proper Python exceptions

* Declare some more exceptions from C code

* Remove special case error message for c parser

* Add whatsnew entry

* Fix missing semicolons

* Add regression test

* Remove special case handling for Windows

PyErr_SetFromErrnoWithFilename works for Unix and Windows

* Remove call to GetLastError(), when using 0, the python error code handles this

* black fixes

* Fix indentation of assert statement (also in previous test, same error)

* Skip the test on windows

* Fix black issue

* Let new_mmap fail without exception to allow fallback

* Do not create a python error in new_mmap to allow the fallback to work silently

* Remove the NULL pointer check for new_rd_source now that it will raise an exception

* Update doc/source/whatsnew/v1.1.0.rst

Co-Authored-By: gfyoung <gfyoung17+GitHub@gmail.com>

Co-authored-by: Jeff Reback <jeff@reback.net>
Co-authored-by: gfyoung <gfyoung17+GitHub@gmail.com>
jbrockmendel pushed a commit to jbrockmendel/pandas that referenced this pull request Mar 23, 2020
…2737)

* Generate exception from the C code in the proper manner

Get rid of all error printf's and produce proper Python exceptions

* Declare some more exceptions from C code

* Remove special case error message for c parser

* Add whatsnew entry

* Fix missing semicolons

* Add regression test

* Remove special case handling for Windows

PyErr_SetFromErrnoWithFilename works for Unix and Windows

* Remove call to GetLastError(), when using 0, the python error code handles this

* black fixes

* Fix indentation of assert statement (also in previous test, same error)

* Skip the test on windows

* Fix black issue

* Let new_mmap fail without exception to allow fallback

* Do not create a python error in new_mmap to allow the fallback to work silently

* Remove the NULL pointer check for new_rd_source now that it will raise an exception

* Update doc/source/whatsnew/v1.1.0.rst

Co-Authored-By: gfyoung <gfyoung17+GitHub@gmail.com>

Co-authored-by: Jeff Reback <jeff@reback.net>
Co-authored-by: gfyoung <gfyoung17+GitHub@gmail.com>
jbrockmendel pushed a commit to jbrockmendel/pandas that referenced this pull request Mar 25, 2020
…2737)

* Generate exception from the C code in the proper manner

Get rid of all error printf's and produce proper Python exceptions

* Declare some more exceptions from C code

* Remove special case error message for c parser

* Add whatsnew entry

* Fix missing semicolons

* Add regression test

* Remove special case handling for Windows

PyErr_SetFromErrnoWithFilename works for Unix and Windows

* Remove call to GetLastError(), when using 0, the python error code handles this

* black fixes

* Fix indentation of assert statement (also in previous test, same error)

* Skip the test on windows

* Fix black issue

* Let new_mmap fail without exception to allow fallback

* Do not create a python error in new_mmap to allow the fallback to work silently

* Remove the NULL pointer check for new_rd_source now that it will raise an exception

* Update doc/source/whatsnew/v1.1.0.rst

Co-Authored-By: gfyoung <gfyoung17+GitHub@gmail.com>

Co-authored-by: Jeff Reback <jeff@reback.net>
Co-authored-by: gfyoung <gfyoung17+GitHub@gmail.com>
@simonjayhawkins simonjayhawkins mentioned this pull request May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Error Reporting Incorrect or improved errors from pandas IO CSV read_csv, to_csv
Projects
None yet
Development

Successfully merging this pull request may close these issues.

read_csv throws wrong exception on permissions issue
5 participants