Skip to content

bpo-41749: Little improve on imghdr#22166

Closed
eamanu wants to merge 7 commits into
python:masterfrom
eamanu:little-improve-imghdr
Closed

bpo-41749: Little improve on imghdr#22166
eamanu wants to merge 7 commits into
python:masterfrom
eamanu:little-improve-imghdr

Conversation

@eamanu
Copy link
Copy Markdown
Contributor

@eamanu eamanu commented Sep 9, 2020

For the what() function, the file parameter is always needed.
In despite of that users can use h for send a bytes stream to
detect the kind of the image, the file parameter is need.

Don't have sense ask for file parameter when this parameter will
not any effect on the result of the what function.

bpo-41749: Little improve on imghdr

https://bugs.python.org/issue41749

For the `what()` function, the `file` parameter is always needed.
In despite of that users can use `h` for send a bytes stream to
detect the kind of the image, the `file` parameter is need.

Don't have sense ask for `file` parameter when this parameter will
not any effect on the result of the `what` function.
@eamanu eamanu changed the title Little improve on imghdr bpo-41749: Little improve on imghdr Sep 9, 2020
@eamanu eamanu closed this Sep 11, 2020
@eamanu eamanu reopened this Sep 11, 2020
Copy link
Copy Markdown
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

I am currently leaning toward rejecting the change, but if it is not, indicated changes are needed.

Comment thread Lib/imghdr.py Outdated
Comment thread Lib/test/test_imghdr.py Outdated
Comment thread Lib/test/test_imghdr.py
with self.assertRaises(TypeError):
imghdr.what(self.testfile, 1)
with self.assertRaises(AttributeError):
with self.assertRaises(BytesWarning):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given that the signature change should not affect this call, I don't understand the error change.

@@ -0,0 +1 @@
Allow to programmer to don't use `file` parameter on `what` on imghdr library. No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This needs revision, but I won't bother unless we decide to apply change.

@bedevere-bot
Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Comment thread Lib/imghdr.py
#-------------------------#

def what(file, h=None):
def what(file='', h=None):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To catch 'what()' properly, the default should be a sentinel that people do not pass. Before this add

Suggested change
def what(file='', h=None):
_none = object()
def what(file=_none, h=None):

Comment thread Lib/imghdr.py
f = None
try:
if h is None:
if file == '':
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if file == '':
if file == _none:

eamanu and others added 2 commits September 12, 2020 22:55
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
@eamanu eamanu closed this Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants