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

2.1.0: filtering all code over pyupgrade --py38-plus causes some issues #451

Open
kloczek opened this issue Apr 28, 2024 · 3 comments
Open

Comments

@kloczek
Copy link

kloczek commented Apr 28, 2024

I'v been trying to cut off EOSed already python<3.7 code by filtering all code over pyupgrade --py38-plus.
Below patch has been generated

--- a/tests/test_integration.py
+++ b/tests/test_integration.py
@@ -149,10 +149,10 @@
     :param z: baz
     """

-    def __init__(self, x: bool, y: int, z: Optional[str] = None) -> None:  # noqa: UP007
+    def __init__(self, x: bool, y: int, z: str | None = None) -> None:  # noqa: UP007
         pass

-    def a_method(self, x: bool, y: int, z: Optional[str] = None) -> str:  # noqa: UP007
+    def a_method(self, x: bool, y: int, z: str | None = None) -> str:  # noqa: UP007
         """
         Method docstring.

@@ -183,7 +183,7 @@
         """

     @classmethod
-    def a_classmethod(cls, x: bool, y: int, z: Optional[str] = None) -> str:  # noqa: UP007
+    def a_classmethod(cls, x: bool, y: int, z: str | None = None) -> str:  # noqa: UP007
         """
         Classmethod docstring.

@@ -193,7 +193,7 @@
         """

     @staticmethod
-    def a_staticmethod(x: bool, y: int, z: Optional[str] = None) -> str:  # noqa: UP007
+    def a_staticmethod(x: bool, y: int, z: str | None = None) -> str:  # noqa: UP007
         """
         Staticmethod docstring.

@@ -271,7 +271,7 @@
       bytes
 """,
 )
-def function(x: bool, y: int, z_: Optional[str] = None) -> str:  # noqa: ARG001, UP007
+def function(x: bool, y: int, z_: str | None = None) -> str:  # noqa: ARG001, UP007
     """
     Function docstring.

@@ -648,7 +648,7 @@
       "None"
 """,
 )
-def func_with_overload(a: Union[int, str], b: Union[int, str]) -> None:  # noqa: ARG001, UP007
+def func_with_overload(a: int | str, b: int | str) -> None:  # noqa: ARG001, UP007
     """
     f does the thing. The arguments can either be ints or strings but they must
     both have the same type.
@@ -676,7 +676,7 @@
 class TestClassAttributeDocs:
     """A class"""

-    code: Optional[CodeType]  # noqa: UP007
+    code: CodeType | None  # noqa: UP007
     """An attribute"""

I'm not sure is something wrong with pyupgrade or sphinx-autodoc-typehints code needs to be prepared to filter it over pyugrade.
I think that it would be good to have look on that because this year Sep 3.8 will be EOSed 🤔

@hoodmane
Copy link
Collaborator

hoodmane commented May 3, 2024

I think the type rendering shows something different depending on whether you put Optional[X] vs X | None. It's not clear how much this is the intended behavior vs how much it's an accident. But I think maybe we should exempt the test files from this pyupgrade rule.

@kloczek
Copy link
Author

kloczek commented May 3, 2024

I do not feel enough competent to help you with that 😢

Just if you see anything wrong in those changes please report that on https://github.com/asottile/pyupgrade/issues.
If it could be sphinx-autodoc-typehints code issue it would be good to clean that here and apply all pyupgrade --py38-plus changes before next release (3.7 has been EOSed ~10 months ago).
Simple it would be good as well to test all changes generated by pyupgrade for next major releases to be able apply those changes instantly when exact next major version will be EOSed (Sep this year will be EOSed 3.8).

But I think maybe we should exempt the test files from this pyupgrade rule.

I'm not sure is it good solution because probably sooner or latter it will be probably necessary to resolve that 🤔

FYI: So far on testing

[tkloczko@pers-jacek SPECS]$ grep -l "Patch:.*%{name}-pyupgrade_py38.patch" *spec | wc -l
327

modules this one is only one with that kind of issues so basing only on that probability that it could be something with pyupgrade is relatively low (however still non-zero). This is why I've decided to open issue ticket here ..

@hoodmane
Copy link
Collaborator

hoodmane commented May 3, 2024

Just if you see anything wrong in those changes please report that on https://github.com/asottile/pyupgrade/issues.

There's nothing wrong with the changes, it's certainly not a pyupgrade bug. Optional[X] and X | None should be identical from the point of view of any type checker. The issue is that they are different Python objects so if you look up the type hints you won't get identical objects. It's the responsibility of programs that do runtime type analysis to handle them identically or not. In our case, in some configurations we handle them differently.

sooner or latter it will be probably necessary to resolve that

This change is not a "you will need to do this eventually or it will break" kind of thing but rather a "we think this is a nicer way to write this that works in this new Python version" change. If you look at the Python docs it says under Optional:

Changed in version 3.10: Optional can now be written as X | None. See union type expressions.

It can be written that way but does not have to be. Optional is not deprecated and they are planning to keep it for the foreseeable future. But maybe we ought to handle X | None and Optional[X] identically I'm not sure.

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

No branches or pull requests

2 participants