-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Do not use percent format in strings #8045
Conversation
created radarhere#25 for the |
src/PIL/PdfParser.py
Outdated
msg = ( | ||
"bad or missing Length in stream dict " | ||
f"({result.get(b'Length')})" | ||
) |
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.
msg = ( | |
"bad or missing Length in stream dict " | |
f"({result.get(b'Length')})" | |
) | |
stream_len = result.get(b"Length") | |
msg = f"bad or missing Length in stream dict ({stream_len})" |
radarhere#25 feels that error message should fit onto one line. The change above this would be my suggestion for that goal.
The suggestion from the PR is
stream_len_str = result.get(b"Length")
try:
stream_len = int(stream_len_str)
except (TypeError, KeyError, ValueError) as e:
msg = f"bad or missing Length in stream dict ({stream_len_str})"
raise PdfFormatError(msg) from e
I think that makes the code harder to read, because, if you ignore the exception, there's more to understand.
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.
My suggestion would be radarhere#25 (comment):
try:
stream_len_str = result.get(b"Length")
stream_len = int(stream_len_str)
except (TypeError, KeyError, ValueError) as e:
msg = f"bad or missing Length in stream dict ({stream_len_str})"
raise PdfFormatError(msg) from e
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.
Ok, I've pushed that - minus the KeyError
, because now that result[b"Length"]
is gone, it will no longer happen. int(None)
will instead raise a TypeError.
Tests/test_image_access.py
Outdated
@@ -415,7 +415,9 @@ def test_embeddable(self) -> None: | |||
|
|||
int main(int argc, char* argv[]) | |||
{ | |||
char *home = "%s"; | |||
char *home = \"""" | |||
+ sys.prefix.replace("\\", "\\\\") |
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.
Can we put this in a variable outside the string, then make this an f-string?
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.
Ok, I've pushed a commit. It did require escaping brackets though.
Fixes the failures in #8044
Converts several percent format strings to f-strings.
There is also a percent format inside a multiline string - I think the neatest solution is just to break the multiline string.