nipy / nibabel

Python package to access a cacophony of neuro-imaging file formats
http://nipy.org/nibabel/
Other
639 stars 257 forks source link

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

Open anibalsolon opened 1 year ago

anibalsolon commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

effigies commented 1 year ago

@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.