imghdr.what doesn't accept bytes paths

[PCManticore]

BPO	19997
Nosy	@pitrou, @vstinner, @PCManticore, @serhiy-storchaka, @vajrasky
Dependencies	bpo-19990: Add unittests for imghdr module
Files	imghdr_bytes.patch imghdr_files.patch imghdr_files_2.patch imghdr_bytes.patch imghdr_bytes_2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2014-08-22.02:07:45.149>
created_at = <Date 2013-12-16.12:48:13.893>
labels = ['type-bug', 'library']
title = "imghdr.what doesn't accept bytes paths"
updated_at = <Date 2014-08-22.02:07:45.147>
user = 'https://github.com/PCManticore'

bugs.python.org fields:

activity = <Date 2014-08-22.02:07:45.147>
actor = 'Claudiu.Popa'
assignee = 'none'
closed = True
closed_date = <Date 2014-08-22.02:07:45.149>
closer = 'Claudiu.Popa'
components = ['Library (Lib)']
creation = <Date 2013-12-16.12:48:13.893>
creator = 'Claudiu.Popa'
dependencies = ['19990']
files = ['33164', '33174', '33195', '33553', '34181']
hgrepos = []
issue_num = 19997
keywords = ['patch']
message_count = 12.0
messages = ['206299', '206394', '206419', '206525', '206526', '206542', '208492', '211916', '211940', '219557', '225605', '225636']
nosy_count = 5.0
nosy_names = ['pitrou', 'vstinner', 'Claudiu.Popa', 'serhiy.storchaka', 'vajrasky']
pr_nums = []
priority = 'normal'
resolution = 'wont fix'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue19997'
versions = ['Python 3.5']

[PCManticore]

imghdr.what check explicitly for string path, while open happily accepts bytes paths, as seen below:

>>> x
b'\xc2\xba'
>>> imghdr.what(x)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/tank/libs/cpython/Lib/imghdr.py", line 15, in what
    location = file.tell()
AttributeError: 'bytes' object has no attribute 'tell'
>>> open(x)
<_io.TextIOWrapper name=b'\xc2\xba' mode='r' encoding='UTF-8'>

A reason why this should be supported can be found in this message: http://bugs.python.org/msg191691.
The following patch fixes this. Also, it depends on bpo-19990 (where test_imghdr.py was added).

[vajrasky]

Why limit to str and bytes. Open can accept integer as well. I am asking not suggesting.

>>> with open('/tmp/cutecat.txt', 'wb') as f:
...   f.write(b'cutecat')
... 
7
>>> a = open('/tmp/cutecat.txt')
>>> a.fileno()
3
>>> b = open(3)
>>> b.read()
'cutecat'

[PCManticore]

Thanks, Vajrasky, I forgot about the fact that open can open file descriptors. Here's a patch which fixes this, also adds a test for BytesIO and uses os.fsencode instead of .encode().

[vstinner]

imghdr_bytes.patch should use os.fsencode() to encode the filename, not filename.encode().

(You may use a valid sound file of the Python suite test instead of generating an invalid sounf file.)

imghdr_files.patch looks overkill. I don't think that it's useful to support integer file descriptor. Being able to pass an open file is enough. imghdr_bytes.patch is fine.

[PCManticore]

Hi, Victor! The second patch does use os.fsencode. I'll update the first one with this change.

[PCManticore]

Added the new version.

(You may use a valid sound file of the Python suite test instead of generating an invalid sounf file.)

Oh, that's sndhdr, currently imghdr doesn't have valid image files in the Python suite test (there is another issue for adding a test file for this module). If it's required, sure, I'll add a few.

[PCManticore]

Updated patch to use real image files from issue bpo-19990.

[PCManticore]

Patch updated. It removes the check added in http://hg.python.org/cpython/rev/94813eab5a58 and simplifies the test for bytes file path.

[serhiy-storchaka]

I'm not sure imghdr.what() should support bytes path. The open() builtin, most os and os.path functions support string and bytes paths, but many other modules (including pathlib) support only string paths.

[PCManticore]

There are other modules with support for bytes filenames in their API:

bz2
codecs
gzip
lzma
pipes.Template
tarfile
tokenize
fileinput
filecmp
sndhdr
configparser

Also, given the fact that sndhdr supports them and its purpose is similar with imghdr, it would be a surprise
for a beginner to find out that imghdr.what(b"img") is not working, while sndhdr.what(b"snd") works.

[serhiy-storchaka]

See also a discussion at Python-Dev:
http://comments.gmane.org/gmane.comp.python.devel/149048

Looks as there are no need to add bytes path support in such high-level API. In any case you can call os.fsdecode() on path argument.

[PCManticore]

Right, I've read the thread you posted and I agree. My use cases aren't strong enough and it's enough for me to call os.fsdecode.