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

really drop python <=3.7 support #1476

Closed
wants to merge 2 commits into from
Closed

Conversation

kloczek
Copy link

@kloczek kloczek commented Jul 2, 2024

Filetrer all cod over pyupgrade --py38-plus.

kloczek added 2 commits July 2, 2024 11:26
Filetrer all cod over `pyupgrade --py38-plus`.

Signed-off-by: Tomasz Kłoczko <[email protected]>
Copy link
Member

@aaugustin aaugustin left a comment

Choose a reason for hiding this comment

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

Some of the changes are valid — e.g. unnecessary imports — but there's also many incorrect changes. Unfortunately it will probably be faster for me to redo the changes than to review and guide you over GitHub comments :-/ I gave up reviewing after a few files because the signal / noise ratio wasn't great.

@@ -25,7 +25,6 @@
copyright = f"2013-{datetime.date.today().year}, Aymeric Augustin and contributors"
author = "Aymeric Augustin"

from websockets.version import tag as version, version as release
Copy link
Member

Choose a reason for hiding this comment

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

That will break generating the docs.

@@ -7,6 +7,7 @@
import django
import websockets


Copy link
Member

Choose a reason for hiding this comment

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

Please avoid gratuitous formatting changes. Also, probably the autoformatter built into the project would revert it at the next commit.

from websockets.server import serve


Copy link
Member

Choose a reason for hiding this comment

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

Again, gratuitous change that will make the docs look worse.

@aaugustin
Copy link
Member

I think I'm going to redo individual changes — again, faster to redo than review — which will eventually lead to the result that you're aiming for. Thank you for the sugestion! That's a good opportunity to clean up.

@kloczek
Copy link
Author

kloczek commented Jul 2, 2024

OK so just cannel this PR and do that your way.
If you see some code wrongly filtered please discuss that with pyupgrade maintainer.
FYI I've filtered websockets code up to --py310-plus and it passes test suite and I don't see any issues on interaction with other modules. code.

@aaugustin
Copy link
Member

Union types were introduced in Python 3.10. See https://peps.python.org/pep-0604/. As a consequence, this change isn't compatible with Python 3.8.

$ python3.8
Python 3.8.16 (default, Apr  2 2023, 07:49:54)
[Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> def foo(bar=str | None): pass
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
>>>
$ python3.10
Python 3.10.10 (main, Apr  2 2023, 07:49:30) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> def foo(bar=str | None): pass
...
>>>

@aaugustin aaugustin closed this Jul 12, 2024
@aaugustin aaugustin reopened this Jul 12, 2024
@aaugustin
Copy link
Member

Oops. Actually not a problem thanks to from __future__ import annotations. I had missed that.

@aaugustin
Copy link
Member

Need to check that the docs still generate properly.

@aaugustin
Copy link
Member

As of f45286b all the good changes from this PR are in the main branch.

@aaugustin aaugustin closed this Jul 12, 2024
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.

2 participants