-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Drop support for EOL Python 3.5 #4794
Conversation
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.
There are a few places where you can use {var!r}
instead of {repr(var)}
, and a few more small issues.
@@ -212,10 +212,10 @@ class DdsImageFile(ImageFile.ImageFile): | |||
def _open(self): | |||
magic, header_size = struct.unpack("<II", self.fp.read(8)) | |||
if header_size != 124: | |||
raise OSError("Unsupported header size %r" % (header_size)) | |||
raise OSError(f"Unsupported header size {repr(header_size)}") |
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.
raise OSError(f"Unsupported header size {repr(header_size)}") | |
raise OSError(f"Unsupported header size {header_size!r}") |
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 for detailed review!
https://stackoverflow.com/a/44800859/724176 says of !r
:
It just calls the
repr
of the value supplied.It's usage is generally not really needed with f-strings since with them you can just do
repr(self.radius)
which is arguably more clear in its intent.
!r
(repr
),!s
(str
) and!a
(ascii
) were kept around just to ease compatibility with thestr.format
alternative, you don't need to use them with f-strings.
I'm tempted to keep the more explicit version, and the f-string PEP says "!s
, !r
, and !a
are redundant".
Maybe we should replace the other handful of existing !r
s with repr
.
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.
Maybe we should replace the other handful of existing
!r
s withrepr
.
Done in last push.
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.
There's one more in Tests/test_imagemath.py
at the top. I personally prefer !r
for brevity, but it's fine as long as it's consistent. Perhaps it would be a good idea to add a lint rule for this?
Co-authored-by: nulano <[email protected]>
Found another little bit to remove. |
Will merge in a bit if there's no further comments! |
For #4764.
Continuation of #4746.
Changes proposed in this pull request:
py36
pyupgrade `git ls-tree -r HEAD --name-only | grep ".py$"` --py36-plus
and manuallyFor clarity, I left some as percent formatters:
And as
.format
:Happy to change any of these too if we think they're better, and if I missed any.
Can also split the PR by, say,
src
andTests
directory if it makes it easier to review. I recommend marking files as viewed.