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

bpo-41749: Little improve on imghdr #22166

Closed
wants to merge 7 commits into from

Conversation

eamanu
Copy link
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
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.

Lib/imghdr.py Outdated Show resolved Hide resolved
Lib/test/test_imghdr.py Outdated Show resolved Hide resolved
imghdr.what()
with self.assertRaises(AttributeError):
imghdr.what(None)
with self.assertRaises(TypeError):
imghdr.what(self.testfile, 1)
with self.assertRaises(AttributeError):
with self.assertRaises(BytesWarning):
Copy link
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.
Copy link
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

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.

@@ -8,10 +8,13 @@
# Recognize image headers #
#-------------------------#

def what(file, h=None):
def what(file='', h=None):
Copy link
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):

f = None
try:
if h is None:
if file == '':
Copy link
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.

None yet

5 participants