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
Change ContainerIO return type to match file object mode #4297
Conversation
src/PIL/ContainerIO.py
Outdated
while True: | ||
c = self.read(1) | ||
if not c: | ||
break | ||
s = s + c | ||
if c == b"\n": | ||
if c in [b"\n", "\n"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line results in a BytesWarning
when running with the Python -b
argument:
Tests/test_file_tar.py::TestFileTar::test_sanity
.../Pillow/.tox/py38/lib/python3.8/site-packages/PIL/ContainerIO.py:101: BytesWarning: Comparison between bytes and string
if c in [b"\n", "\n"]:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I've updated the commit.
Overall, the approach makes sense. Do you have a use case in mind where the file would be opened in text mode? |
No. Feel free to argue that bytes mode is correct. I'm just thinking about backwards compatibility. |
src/PIL/ContainerIO.py
Outdated
while True: | ||
c = self.read(1) | ||
if not c: | ||
break | ||
s = s + c | ||
if c == "\n": | ||
if c == (b"\n" if "b" in self.fh.mode else "\n"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be pulled out of the loop so it doesn't need to be recalculated every iteration of the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I've pushed a commit for this.
Image data is expected to be read in bytes mode, not text mode so ContainerIO should return bytes in all methods. The passed in file handler is expected to be opened in bytes mode (as TarIO already does).
Alternative to #4192
That PR changes ContainerIO to return bytes instead of strings.
This PR changes ContainerIO to return strings if the file object is a string object, and bytes if the file object is a bytestring object.