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

Experimental bytes support #15

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Experimental bytes support #15

wants to merge 1 commit into from

Conversation

thcrock
Copy link

@thcrock thcrock commented Apr 4, 2019

This isn't complete, at the very least in that the docs still have to be updated. I wanted to get feedback on this approach to adding bytestream support

@thcrock thcrock requested a review from jesteria April 4, 2019 22:28
return '\n'


class StreamBufferedIOBase(StreamIOBase, io.BufferedIOBase):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing it, but is there a reason for the change in base class? It seems like it could still inherit TextIOBase.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just how Python describes working with the different types of streams. https://docs.python.org/3/library/io.html#text-i-o

I didn't try and test TextIO as a base class, but I didn't see any reason to deviate from the docs if we're working with bytes here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I reviewed the docs too, just didn't think it mattered. But thanks, I see what you mean now.

I had been meaning to play around with stuff like that, namely giving it a base of say IOBase (rather than TextIOBase) – #3. Really, it's just a pass-through, so it doesn't seem like it should care either way; (and, that's why I like the lazy type detection version, discussed below).


@staticmethod
def _get_newline():
return '\n'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't the interface be condensed?

class StreamIOBase(io.TextIOBase):

    def __init__(self):
        self._remainder = self.constructor()

    # (etc)

class StreamTextIOBase(StreamIOBase):

    constructor = str
    newline = '\n'


@staticmethod
def _get_newline():
return '\n'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there would be utility in keeping it even more condensed than that however.

class StreamTextIOBase(io.TextIOBase):

    def __init__(self, buffer_cls=str):
        self._buffer_cls = buffer_cls

        if self._buffer_cls is str:
            self._newline = '\n'
        elif self._buffer_cls is bytes:
            self._newline = b'\n'
        else:
            raise TypeError("StreamTextIOBase supports buffers of type 'str' or 'bytes' not: %r" % self._buffer_cls)

        self._remainder = self._buffer_cls()

    …

Not only would that be less boilerplate here, then also perhaps it would be simpler down the line:

class IteratorTextIO(baseio.StreamTextIOBase):

    def __init__(self, iterable, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.__iterator__ = iter(iterable)

…

And in your bytes iterator case, just: my_iterator = IteratorTextIO(my_iterable, bytes).

@@ -101,3 +101,98 @@ def test_write_methods(self, buffer, method_name, method_args):

with pytest.raises(io.UnsupportedOperation):
method(*method_args)


class TestIteratorBufferedIO:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And since a lot of this is repetition, I would hope that condensing the base classes would mean we could get away with just a couple extra tests on the original class, to test just the added branches/cases.

@jesteria
Copy link
Member

jesteria commented Apr 5, 2019

Actually, not to bikeshed, but I think we can keep this entirely internal; (and, the best interface is none at all 😸 ).

class StreamTextIOBase(io.TextIOBase):

    def __init__(self):
        self._remainder = None
        self._buffer_cls = None
        self._newline = None

    # yadda yadda yadda

    def read(self, size=None):
        if self.closed:
            raise IOClosed()

        if self._buffer_cls is None:
            self._infer_buffer()

        if size is not None and size < 0:
            size = None

        result = self._buffer_cls()

        # etc.....

    def readline(self):
        if self.closed:
            raise IOClosed()

        if self._buffer_cls is None:
            self._infer_buffer()

        result = self._buffer_cls()

        # etc....

    def _infer_buffer(self):
        try:
            self._remainder = self.__next_chunk__()
        except StopIteration:
            self._remainder = ''

        self._buffer_cls = type(self._remainder)

        if isinstance(self._remainder, str):
            self._newline = '\n'
        elif isinstance(self._remainder, bytes):
            self._newline = b'\n'
        else:
            raise TypeError("StreamTextIOBase supports text of type 'str' or 'bytes' not: %r" % self._buffer_cls)

I think, with changes like the above, we'd otherwise need a test case or two to ensure it works with bytes; but, that's all, and users could just use it without worrying about bytes vs. str.

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

Successfully merging this pull request may close these issues.

2 participants