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

Drop support for EOL Python 3.5 #4794

Merged
merged 6 commits into from Sep 2, 2020
Merged

Drop support for EOL Python 3.5 #4794

merged 6 commits into from Sep 2, 2020

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Jul 16, 2020

For #4764.

Continuation of #4746.

Changes proposed in this pull request:

  • Update Black target to py36
  • Upgrade Python syntax for 3.6+ using pyupgrade `git ls-tree -r HEAD --name-only | grep ".py$"` --py36-plus and manually

For clarity, I left some as percent formatters:

    print("Size: %sx%s" % im.size)
...
            raise OSError("Unsupported BMP Size: (%dx%d)" % self.size)
...
            imf.save("out-%s-%s-%s.png" % size)
...
            page_contents = b"q %d 0 0 %d 0 0 cm /image Do Q\n" % (
                int(width * 72.0 / resolution),
                int(height * 72.0 / resolution),
            )

And as .format:

        print(
            "{}: {:.4f} s  {:.6f} per iteration".format(
                label, endtime - starttime, (endtime - starttime) / (x + 1.0)
            )
...
                    tga_path = "{}_{}_{}.tga".format(
                        path_no_ext, origin, "rle" if rle else "raw"
                    )

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 and Tests directory if it makes it easier to review. I recommend marking files as viewed
.

@hugovk hugovk added the Removal Removal of a feature, usually done in major releases label Jul 16, 2020
Copy link
Contributor

@nulano nulano left a 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)}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise OSError(f"Unsupported header size {repr(header_size)}")
raise OSError(f"Unsupported header size {header_size!r}")

Copy link
Member Author

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 the str.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 !rs with repr.

Copy link
Member Author

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 !rs with repr.

Done in last push.

Copy link
Contributor

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?

Tests/test_image_resample.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
src/PIL/BlpImagePlugin.py Show resolved Hide resolved
src/PIL/PngImagePlugin.py Show resolved Hide resolved
src/PIL/PngImagePlugin.py Show resolved Hide resolved
src/PIL/PngImagePlugin.py Show resolved Hide resolved
src/PIL/TiffImagePlugin.py Show resolved Hide resolved
src/PIL/TiffImagePlugin.py Show resolved Hide resolved
Co-authored-by: nulano <nulano@nulano.eu>
@hugovk
Copy link
Member Author

hugovk commented Aug 11, 2020

Found another little bit to remove.

@hugovk
Copy link
Member Author

hugovk commented Sep 1, 2020

Will merge in a bit if there's no further comments!

@hugovk hugovk merged commit 5ce2fac into python-pillow:master Sep 2, 2020
@hugovk hugovk deleted the rm-3.5 branch September 2, 2020 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup Needs Rebase Removal Removal of a feature, usually done in major releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants