Skip to content

ftpfs possible issue with openbin #406

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

Closed
al8ba opened this issue Jun 24, 2020 · 5 comments · Fixed by #435
Closed

ftpfs possible issue with openbin #406

al8ba opened this issue Jun 24, 2020 · 5 comments · Fixed by #435
Labels
Milestone

Comments

@al8ba
Copy link

al8ba commented Jun 24, 2020

Recently, while trying out the ftpfs, I've noticed that to open a binary file-like object for writing by some third-party packages I have to do this:

with open_fs("ftp://anonymous:anonymous@localhost:8021") as fs:
    with fs.openbin("test.bin", "wb") as f:
        ...

Note in the above the mode param used in openbin includes b: Looking at the docs for the mode param it suggests the b is implied via openbin, but b isn't implicitly added to the mode of the file-like that is returned by openbin - this means that, unless b is explicitly provided as above, third-party packages that inspect the mode of the file-like may complain that it is not opened in binary mode.

Should openbin not reflect the implied b by implicitly adding b to the mode?

@lurch
Copy link
Contributor

lurch commented Jun 24, 2020

Yeah, perhaps FTPFS should also be doing this? https://github.com/PyFilesystem/pyfilesystem2/blob/master/fs/osfs.py#L360

@al8ba
Copy link
Author

al8ba commented Jun 24, 2020

Yes, I was thinking of mentioning use of to_platform_bin in a similar manner.

I guess another way of putting the question is that, given the "b is implied by openbin" in the docs, then for a file f opened via it should "b" in f.mode hold true? Sounds almost like a unit test...

@willmcgugan
Copy link
Member

The base class only guarantees that openbin will return a BinaryIO, which doesn't contain a "mode" attribute. Only the io.FileIO class can be sure of having mode.

So it would be a mistake to assume that all file-like objects have a "mode" attribute. But people do write broken code because they make reasonable assumptions and the specifications are too difficult to understand, so perhaps it would be a good idea for ftp files to add a mode attribute.

@al8ba
Copy link
Author

al8ba commented Jun 24, 2020

so perhaps it would be a good idea for ftp files to add a mode attribute.

The FTPFile object does have a mode attribute - the issue I'm bringing up is that if the FTPFile instance comes from a call to openbin then the mode within it will not contain b, unless b was explicitly supplied in the call to openbin.

I think a solution to this, similar to the previous note about osfs, would be to have the final two lines of openbin look more akin to this:

        ...
        ftp_file = FTPFile(self, _path, mode.to_platform_bin())
    return ftp_file  # type: ignore

@althonos
Copy link
Member

Actually, it is reasonable to expect FS.openbin to return an object with a mode attribute (containing a "b" flag), since it all filesystems are doing it but FTPFile and _MemoryFile. I patched them in #435

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 a pull request may close this issue.

4 participants