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

Python 3 style using pyupgrade #3638

Merged
merged 4 commits into from
Nov 25, 2022
Merged

Python 3 style using pyupgrade #3638

merged 4 commits into from
Nov 25, 2022

Conversation

mwtoews
Copy link
Contributor

@mwtoews mwtoews commented Nov 22, 2022

Overview

This upgrades a few Python 3 styles, automated using pyupgrade, using the command:

$ pyupgrade --py37-plus `find . -name "*.py*"`

with a few manual edits for quality control.

Details

  • Use "yield from" where possible
  • Use f-strings where possible
  • Updates to python modules faulthandler and collections.abc
  • No need to inherit classes from object anymore
  • IOError is an alias for OSError
  • Correction to use a raw string r'doctest: *\+SKIP'); I believe "noqa: W605" was incorrect and should not have been ignored
  • Remove unnecessary parentheses
  • Use set comprehension and literals

If any of these styles are objectionable and the current is preferred, note these and I'll restore them.

@github-actions github-actions bot added the documentation Anything related to the documentation/website label Nov 22, 2022
@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Merging #3638 (4e06e19) into main (728a71f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3638   +/-   ##
=======================================
  Coverage   95.11%   95.12%           
=======================================
  Files          83       83           
  Lines       18560    18556    -4     
=======================================
- Hits        17654    17652    -2     
+ Misses        906      904    -2     

Copy link
Member

@adeak adeak left a comment

Choose a reason for hiding this comment

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

Good catch on most of these, thanks. I only have a few remarks.

pyvista/plotting/widgets.py Show resolved Hide resolved
pyvista/core/datasetattributes.py Show resolved Hide resolved
pyvista/core/filters/poly_data.py Outdated Show resolved Hide resolved
@tkoyama010 tkoyama010 added the maintenance Low-impact maintenance activity label Nov 22, 2022
@mwtoews
Copy link
Contributor Author

mwtoews commented Nov 22, 2022

I've added a pre-commit for pyupgrade --py37-plus --keep-runtime-typing. Another list comprehension to generator conversion was needed. The option --keep-runtime-typing was added to keep current typing, as I'm unsure of these changes. Anyone that wants to investigate, it only impacts one file:

$ pyupgrade --py37-plus pyvista/plotting/colors.py
git diff
diff --git a/pyvista/plotting/colors.py b/pyvista/plotting/colors.py
index 6903e34f..86f4713e 100644
--- a/pyvista/plotting/colors.py
+++ b/pyvista/plotting/colors.py
@@ -460,10 +460,10 @@ class Color:
 
     def __init__(
         self,
-        color: Optional[color_like] = None,
-        opacity: Optional[Union[int, float, str]] = None,
-        default_color: Optional[color_like] = None,
-        default_opacity: Union[int, float, str] = 255,
+        color: color_like | None = None,
+        opacity: int | float | str | None = None,
+        default_color: color_like | None = None,
+        default_opacity: int | float | str = 255,
     ):
         """Initialize new instance."""
         self._red, self._green, self._blue, self._opacity = 0, 0, 0, 0
@@ -543,7 +543,7 @@ class Color:
         return h
 
     @staticmethod
-    def convert_color_channel(val: Union[int, np.integer, float, np.floating, str]) -> int:
+    def convert_color_channel(val: int | np.integer | float | np.floating | str) -> int:
         """Convert the given color channel value to the integer representation.
 
         Parameters
@@ -629,7 +629,7 @@ class Color:
                 raise ValueError(f"Invalid color name or hex string: {arg}") from None
 
     @property
-    def int_rgba(self) -> Tuple[int, int, int, int]:
+    def int_rgba(self) -> tuple[int, int, int, int]:
         """Get the color value as an RGBA integer tuple.
 
         Examples
@@ -655,7 +655,7 @@ class Color:
         return self._red, self._green, self._blue, self._opacity
 
     @property
-    def int_rgb(self) -> Tuple[int, int, int]:
+    def int_rgb(self) -> tuple[int, int, int]:
         """Get the color value as an RGB integer tuple.
 
         Examples
@@ -681,7 +681,7 @@ class Color:
         return self.int_rgba[:3]
 
     @property
-    def float_rgba(self) -> Tuple[float, float, float, float]:
+    def float_rgba(self) -> tuple[float, float, float, float]:
         """Get the color value as an RGBA float tuple.
 
         Examples
@@ -707,7 +707,7 @@ class Color:
         return self._red / 255.0, self._green / 255.0, self._blue / 255.0, self._opacity / 255.0
 
     @property
-    def float_rgb(self) -> Tuple[float, float, float]:
+    def float_rgb(self) -> tuple[float, float, float]:
         """Get the color value as an RGB float tuple.
 
         Examples
@@ -787,7 +787,7 @@ class Color:
         return self.hex_rgba[:-2]
 
     @property
-    def name(self) -> Optional[str]:
+    def name(self) -> str | None:
         """Get the color name.
 
         Returns
if these changes are good, then they can be committed and that arg can be removed from the pre-commit.

@adeak
Copy link
Member

adeak commented Nov 22, 2022

@mwtoews I'm not quite sure how I feel about that diff about runtime typing. If I understand correctly, tuple[float, float, float] needs 3.9 and int | None needs 3.10, and the only reason this all works is that the module in question has from __future__ import annotations which was introduced in 3.7.0b1, so these expressions are never evaluated when the library is imported or run. I don't know, for instance, how typing tools such as mypy consider python versions. Do they make assumptions about the python version they are installed in? Because then a 3.7 system (which we support) would not be able to correctly typecheck these annotations. Is this the case, or is this safe?

In other words, what are the exact risks associated with --keep-runtime-typing?

@mwtoews
Copy link
Contributor Author

mwtoews commented Nov 22, 2022

@adeak the way I understand it is that --keep-runtime-typing essentially ignores most (all?) typing changes related to PEP 585 and PEP 604. I've been using Python for a few decades, but have avoided getting involved with annotations as it seems to change each Python release. I wouldn't know which runtime typing is correct. But it seems that pyupgrade is not honouring --py37-plus w.r.t. the runtime typing changes, so this is possibly a bug. This is a good reason to keep --keep-runtime-typing for now.

Copy link
Member

@adeak adeak left a comment

Choose a reason for hiding this comment

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

Great work, thanks a lot @mwtoews!

@adeak adeak mentioned this pull request Nov 23, 2022
4 tasks
@adeak adeak merged commit 8c3f6d9 into pyvista:main Nov 25, 2022
adeak added a commit to adeak/pyvista that referenced this pull request Nov 25, 2022
* upstream/main:
  Python 3 style using pyupgrade (pyvista#3638)
  Move google_analytics_id to analytics in pydata-sphinx-theme (pyvista#3634)
@mwtoews mwtoews deleted the pyupgrade branch November 25, 2022 22:39
@banesullivan banesullivan mentioned this pull request Feb 1, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Anything related to the documentation/website maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants