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

Remove Python 2 support #1715

Merged
merged 46 commits into from
Jun 15, 2020
Merged

Remove Python 2 support #1715

merged 46 commits into from
Jun 15, 2020

Conversation

septatrix
Copy link
Contributor

This pull requests removes several Python 2 specific code segments and workarounds.
Additionally the docs were updated to state that only Python 3.5+ is supported.
The current changes were done according to the checklist in #1538 and a few search queries over the codebase for version checks etc.
This pull request also removes a few checks for version < 3.5 as these are not supported anymore and I thought this would be a good time to to this as I was searching for them anyways. If this is not desired it can be reversed.

This pull request does not refactor any code to use more Python 3 only features as this would require manual checking of the entire codebase for places where such improvements could be made.

@septatrix septatrix mentioned this pull request Jan 19, 2020
14 tasks
@Bibo-Joshi Bibo-Joshi added this to the 13.0 milestone Jan 19, 2020
@Bibo-Joshi
Copy link
Member

Thanks for the PR!
Please note that Py2 deprecation is "only" due for v13.0, so it might take a while for us to get back to you. But we appreciate your effort :)

@Poolitzer
Copy link
Member

Poolitzer commented Jan 26, 2020

a heads up here: We decided to remove 2.7 from our testing workflow already to reduce testing time.

#1731

Edit: We also decided to remove the support from the public endpoints so users shouldnt get confused anymore
#1734

@septatrix
Copy link
Contributor Author

Currently I do not keep close track of new additions to the code. Therefore I need to go over this again and perform a few changes at some places (e.g. I saw a few cases of old style classes and python 2 version checks).

If there is an expected release date for v13 I will take some time before it to review the code and check for places needing changes.

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Hi! I finally got around to have a look at your PR. Thanks again for you work!

Overall looks good to me. I left a few minor comments/questions. Of course, master would need to be merged. Would you care to do that?

Also, just a thought: Have you run pythons 2to3 on the lib to double check? But imo, it's okay, if we miss some py2 workarounds. We can always remove them, when we find them later on …

telegram/files/sticker.py Outdated Show resolved Hide resolved
telegram/passport/encryptedpassportelement.py Show resolved Hide resolved
telegram/utils/helpers.py Show resolved Hide resolved
tests/test_helpers.py Show resolved Hide resolved
tests/test_user.py Show resolved Hide resolved
@septatrix
Copy link
Contributor Author

Currently I have a few other things to do but I will surely find some spare time to merge master ;)

Yes I ran 2to3 and IIRC another converter though I have not rerun them after the latest commits merging master in here (also why I missed on of the super() you marked). I will have to go through all the new code again and look for unnecessary changes.

Probably should also squash some of the commits merging master / rebase this or will this get done when this gets merged anyhow? I am not sure how you handle this and I know that rewriting history is generally not the best idea...

@Bibo-Joshi
Copy link
Member

Currently I have a few other things to do but I will surely find some spare time to merge master ;)

That'd be great :) Take your time!

Probably should also squash some of the commits merging master / rebase this or will this get done when this gets merged anyhow? I am not sure how you handle this and I know that rewriting history is generally not the best idea...

We'll squash on merge, so no need for sqaushing yourself

@Bibo-Joshi
Copy link
Member

Hi. Just wanted to leave a kind ping :) If you I see that you won't get around to merge master in the forseeable future, feel free to comment. Then I'll try and do that ;)

@septatrix
Copy link
Contributor Author

septatrix commented Jun 10, 2020 via email

@septatrix septatrix marked this pull request as ready for review June 12, 2020 22:57
@septatrix
Copy link
Contributor Author

septatrix commented Jun 13, 2020

Fix #1538

Failing test is test_set_game_score_1 for 3.5 and a timeout (?) for 3.8

@Bibo-Joshi
Copy link
Member

Thanks for the update!

Failing test is test_set_game_score_1 for 3.5 and a timeout (?) for 3.8

No need to worry. test_set_game_score is a b*** and timeout sometimes happen …

Just one more question about the r-strings & related stuff: Did some tests fail for you or did pyupgrade that for you? Just double checking as those gave me headache on the API 4.8 update and I don't want things to break :D

@septatrix
Copy link
Contributor Author

Pyupgrade did those for me - I just did the line breaks manually

@Bibo-Joshi
Copy link
Member

Pyupgrade did those for me - I just did the line breaks manually

Okay, then. Merging. Thanks a lot for your contribution!

@Bibo-Joshi Bibo-Joshi merged commit 8406889 into python-telegram-bot:master Jun 15, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants