Skip to content

Python 3.9 upgrade #24980

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

Merged
merged 3 commits into from
Jan 21, 2023
Merged

Python 3.9 upgrade #24980

merged 3 commits into from
Jan 21, 2023

Conversation

greglucas
Copy link
Contributor

PR Summary

Now that we are on Python 3.9+ (#24919), we can update/simplify some of the code.

Automated command line utility for it:

pyupgrade --py39-plus `find . -name "*.py" -type f`

A lot of this is f-string updates, so I added the first commit to the git ignore blame so there is less churn in the blame lookups.
There are also some OSError aliases and getting rid of the unnecessary parentheses in the lru_cache decorator

PR Checklist

Documentation and Tests

  • [N/A] Has pytest style unit tests (and pytest passes)
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • [N/A] New plotting related features are documented with examples.

Release Notes

  • [N/A] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • [N/A] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • [N/A] Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@greglucas
Copy link
Contributor Author

pre-commit.ci autofix

"/{name}{{{bbox} sc\n".format(
name=font.get_glyph_name(glyph_id),
bbox=" ".join(map(str, [g.horiAdvance, 0, *g.bbox])),
)
Copy link
Member

Choose a reason for hiding this comment

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

I think that one can get f-strings here as well (although those will be horrible...) if it is executed again?

@oscargus
Copy link
Member

oscargus commented Jan 14, 2023

In general, I'm tempting to skip the f-string conversion and let it evolve "organically" as some of them are quite painful as @ksunden points out (others are obviously a clear improvement). Also, some that are turned into format should probably be f-strings (others not).

(Interesting that some unpacking comprehensions are converted into tuples, but not things like if a in ["b", "c"]:.)

Edit: to be clear, things that are not f-string conversions, I do not object to.

@@ -231,7 +231,7 @@ def _check_versions():

# The decorator ensures this always returns the same handler (and it is only
# attached once).
@functools.lru_cache()
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these should be converted to functions.cache for clarity, in fact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated some of them where I thought an unbounded cache was alright, but others I figured we can handle later if we want to change to unbounded rather than the default size of 128. When I was looking through the lru_cache locations it looked like we may even have some decorated instance methods with self that may be keeping instances alive, I wonder if that is causing any of the slowly growing memory leaks that have been reported...

Copy link
Contributor

Choose a reason for hiding this comment

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

I was specifically referring to cases where the cache just serves to establish a singleton, like here.

@greglucas
Copy link
Contributor Author

I went through every .format() call and subjectively evaluated whether f-strings would be nicer or not. So, this is definitely most of the locations that can/should be updated now (even more than the automatic formatter picked up the first go-around). I personally prefer to just get formatting changes updated in one go so you can ignore that commit more easily through blames.

@greglucas greglucas force-pushed the py39-upgrade branch 2 times, most recently from 89c996d to 5c9d97e Compare January 17, 2023 04:44
@timhoffm
Copy link
Member

I think it would be a good idea to add a .git-blame-ignore-revs file. See also https://www.stefanjudis.com/today-i-learned/how-to-exclude-commits-from-git-blame/

@greglucas
Copy link
Contributor Author

I think it would be a good idea to add a .git-blame-ignore-revs file. See also https://www.stefanjudis.com/today-i-learned/how-to-exclude-commits-from-git-blame/

Yes, I agree and we already have one that I updated here :)
https://github.com/matplotlib/matplotlib/pull/24980/files#diff-f247fbfbe928b907db42554d0b3006b28dd11c25a59be031abda73b11a2c934a

ksunden
ksunden previously approved these changes Jan 19, 2023
@ksunden ksunden added this to the v3.8.0 milestone Jan 19, 2023
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

This is good; I can drop my f-string stash now.

@greglucas
Copy link
Contributor Author

Thanks, @QuLogic, good recommendations as usual!

@ksunden ksunden dismissed their stale review January 20, 2023 18:06

conversations in setup.py comments

Command: `pyupgrade --py39-plus`

Manual updates to add some f-strings and ignore f-strings in tex
formats.
@ksunden ksunden merged commit 6a9a071 into matplotlib:main Jan 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants