-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
ENH: support SpooledTemporaryFile #44761
Conversation
twoertwein
commented
Dec 4, 2021
•
edited
Loading
edited
- closes BUG: pd.read_csv - encoding argument ignored for SpooledTemporaryFile #44748
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
@@ -935,7 +930,7 @@ def __init__( | |||
self.decode = decode | |||
|
|||
self.attributes = {} | |||
for attribute in ("seekable", "readable", "writeable"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have been "writable"
but it seems it was never used as we do not support writing to memory-mapped files.
A trimmed down version of this PR could go to 1.3.* as it technically fixes a regression. Let me know whether it should be back-ported or not, based on that I will create a whatsnew for either 1.3 or 1.4. |
let's just put in 1.4, trying to avoid backporting anything unless its critical |
Thank you for rebasing @jreback, green except for (unrelated?) codecov error. |
pandas/io/common.py
Outdated
# seek/read/writ-able. | ||
def __init__(self, buffer: BaseBuffer): | ||
self.buffer = buffer | ||
self.attributes = tuple( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make these separate properties and just do the check of the buffer inside them? its a bit longer code but should be more obvious what is going on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, changed it. I made them methods and not properties, as typing.IO
defines them as methods:
https://github.com/python/typeshed/blob/1d5857e1e0e0fe6078e563d769f07fe3a3352cc9/stdlib/typing.pyi#L522
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sure
thanks @twoertwein this looks great (yeah a bit longer but way more readable). waiting to merge as i hope some CI patches get us back to green and then can rebase here. |
are the windows checks related? |
It should be unrelated (the test case doesn't seem to open any files). Will rebase and force push to test it again. Cannot reproduce it on linux. |
The error disappeared but I can imagine that it might be related: if mmap fails, opened handles will not be released. edit: not from this PR but as a result from #44777 |
thanks @twoertwein |