REF: move get_filepath_buffer into get_handle

[twoertwein]

• closes BUG:Cannot write as xlsx to GCS #33987
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff

This PR makes get_filepath_buffer a private function and is called inside get_handle - one function to rule all of IO ;)

This PR will make it easier for future PRs to make the IO-related interface of read/to_* more consistent as most of them should support compression/memory mapping (for reading)/(binary) file handles/storage options.

Notes to keep track of future follow-up PRs:

• context manager for get_handle
• storage_options for to_excel

[jreback]

wow this is a quite a lot of simplification. some questions and a couple requests for followup (don't add to this PR)

[jreback]

how does this work?

[twoertwein]

yeah, that isn't nice (especially from a typing perspective). The parsers seems to cope with objects implementing __next__ (for example lists). I think a solution would be to have a new attribute for the parser (maybe self.iterable : Optional[Iteratable]). If it is None, use self.handles otherwise use the iterable?

[twoertwein]

this branch is only used by read_excel and is only used for the PythonParser. I typed src and added comments about this special case.

I will try to fix this mess later this week.

[twoertwein]

I think the 'best' solution is to temporarily silence mypy and put a list in IOHandles. PythonParser (the only parser affected by that) had already a check whether the 'file handle' has readline, if not (for the list from read_excel) it treats it as raw data.

edit:
I found a better solution (well-typed and call get_handle after all potential raise(...) calls to avoid leaking file handles)

[pep8speaks]

Hello @twoertwein! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-11-13 06:00:27 UTC

[twoertwein]

I made IOHandles a context manager in the second commit. I'm happy to remove this commit and make it a follow up PR. Unfortunately, we cannot use the context manager in all places. The file handles are often created in a constructor. I think there are a few cases were get_handle could be moved up to to/read_ which would allow us to use a context manager.

[twoertwein]

I haven't yet found an object that needs an explicit mode

[twoertwein]

I don't know why that is done. The file handle should always be in binary mode. If it wasn't in binary mode read would return a string which BytesIO would complain about. Maybe there is a performance reason (seeking faster in BytesIO than the provided buffer, just speculating)?

[jreback]

wow, this is way cleaner that before even. once more merge master and ping on green.

[jreback]

thanks @twoertwein very nice!

[arw2019]

@twoertwein I have a question about the code you added in this PR. In #31817 I'm adding a new csv reader engine (pyarrow) to which I have to provide BytesIO (StringIO is not accepted). I currently have a wrapper to handle this conversion:

class BytesIOWrapper:
    """
    Allows the pyarrow engine for read_csv() to read from string buffers
    """

    def __init__(self, string_buffer: StringIO, encoding: str = "utf-8"):
        self.string_buffer = string_buffer
        self.encoding = encoding

    def __getattr__(self, attr: str):
        return getattr(self.string_buffer, attr)

    def read(self, size: int = -1):
        content = self.string_buffer.read(size)
        return content.encode(self.encoding)

but I'm wondering if this can be done using pandas.io.common.get_handle? I have tried but couldn't quite get it to work. Is it possible with the code as is? If it's not is it something worth adding to get_handle?

Thanks so much!

[twoertwein]

Yes, get_handle would be a good place for this and you probably wouldn't even need any new arguments (is_text and mode might be sufficient).

Towards the end of get_handle there is code for the opposite (wrap binary handles in a TextIOWapper). Adding an elif-block with your wrapper after the TextIOWrapper would be a good place. On the other side, it could also be beneficial to add your wrapper for any text handle(?) before applying compression as we currently do not support compression for text handles.

I assume that the following code might get you quite far

if not (is_text or _is_binary_mode(handle, mode)):
    handle = BytesIOWrapper(handle, encoding) 
    mode = mode.replace("b", "")

[twoertwein]

@arw2019 I think that the BytesIOWrapper is on its own already really useful: I think many doc-strings state that the function accepts any file handle but in fact it is in most cases either text or a binary handle.

About adding the BytesIOWrapper to get_handle: you probbly also need to make sure that the wrapper is itself not added to created_handles, otherwise close will be called on the wrapper which then will call close on the underlying buffer. If the buffer is provided by a user, we shouldn't close it. If the buffer is created by us, it is already in created_handle and will be closed. Any easier solution might be to make close a no-op, then you (or a later part in get_handle) can add it to created_handles.

[arw2019]

@twoertwein Thank you so much for these responses!!!

I'm thinking I'll do a separate PR to add the BytesIOWrapper class and integrate it into get_handle (as #31817 is already quite bloated). I'll cc you on that and would love your feedback if you have time to look!