Skip to content

Drop support for EOL Python 3.5#4794

Merged
hugovk merged 6 commits intopython-pillow:masterfrom
hugovk:rm-3.5
Sep 2, 2020
Merged

Drop support for EOL Python 3.5#4794
hugovk merged 6 commits intopython-pillow:masterfrom
hugovk:rm-3.5

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.

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?

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.

3 participants