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

nibabel.openers.Opener does not guarantee file mode when provided with a file handler #1143

Open
anibalsolon opened this issue Oct 5, 2022 · 3 comments

Comments

@anibalsolon
Copy link
Collaborator

Following the discussion from #1140 with @effigies , the Opener does not guarantee the file mode (or err) when a file handler is provided instead of a filename. E.g.:

fd = open('file', 'r')
with Opener(fd, 'w') as bigO:
    bigO.write('hello')  # trying to write in read mode

fd = open('file', 'r')
with Opener(fd, 'rb') as bigO:
    assert bigO.read(5) == b'hello'  # 'hello' != b'hello'

For read/write, it seems to me that it is no-brainer- just check for the correct chars in the .mode and err when it is different. This change in the contract may require a DeprecationWarning.

Now for text/binary mode, let me know if that's too hacky: When opening in text mode on Py3, the file object has an attribute 'buffer' which is the same as opening the file in binary mode. So something like this would guarantee to always be binary (when requested + for the default mode):

class Opener(object):
    def __init__(self, fileish, *args, **kwargs):
        mode = kwargs['mode']  # all that kwargs processing
        if self._is_fileobj(fileish):
            # binary mode is requested, but fileish mode is text
            if 'b' in mode and 'b' not in fileish.mode:
                self.fobj = fileish.buffer
            else:
                self.fobj = fileish
            ...

fd = open('file', 'r')
with Opener(fd, 'rb') as bigO:
    assert bigO.read(5) == 'hello'  # b'hello' == b'hello'

however, if fd is binary mode and Opener requests text mode, probably a TextIOWrapper needs to be used to convert between modes.

@effigies
Copy link
Member

effigies commented Jan 3, 2023

Hi @anibalsolon, just coming round to a bunch of nibabel issues. I would be up for making the deprecationwarning/futurewarning changes needed now to speed up the transition.

My concern with trying to change the mode is that we might get things other than regular filehandles that provide an IO interface. TextIOBase.buffer is considered an implementation detail and not an API, so I would be hesitant to count on it. Instinctively, I would be more inclined to just error if the mode doesn't match.

@anibalsolon
Copy link
Collaborator Author

Good point about the impl detail, erring seem to be the way

@effigies
Copy link
Member

@anibalsolon I wonder if we should consider accepting fsspecs rather than working more on the Opener. I'm okay with fixing bugs when they arise, but if we're looking to engineer something that Does It Right, it might be wiser to transition to a tool that many other projects are depending on to Do It Right.

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

No branches or pull requests

2 participants