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

sopel: drop support for Python < 3.6 #2062

Merged
merged 16 commits into from
Jun 27, 2021
Merged

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented May 7, 2021

Description

Fix #1801 and hopefully nothing else so @dgw can have a good time removing deprecated stuffs himself. 😁

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

@Exirel Exirel added this to the 8.0.0 milestone May 7, 2021
@lgtm-com

This comment has been minimized.

@dgw
Copy link
Member

dgw commented May 7, 2021

This will have to drop CI for the older Pythons too. Not going to worry about it until 7.1 is done.

LGTM's new alerts are interesting. Annoying, but interesting. I thought we were importing division from the future to avoid such things.

@Exirel
Copy link
Contributor Author

Exirel commented May 8, 2021

This will have to drop CI for the older Pythons too. Not going to worry about it until 7.1 is done.

There are many other things to do, yes. In this PR, I focused only on removing/adapting the code, but I tried not to touch anything that would be used by the CI or the githook stuff.

I thought we were importing division from the future to avoid such things.

Which is not necessary with Python 3 as the true division is now by default. I'm pretty sure it's because LGTM is configured with Py2.7.

Also, It's funny to see that the CI fails on Py2.7 to 3.5, and is fine for 3.6+—just as planned!

@dgw
Copy link
Member

dgw commented May 8, 2021

I tried not to touch anything that would be used by the CI or the githook stuff.

Also, It's funny to see that the CI fails on Py2.7 to 3.5, and is fine for 3.6+—just as planned!

Leaving the CI stuff, at least, means this PR can never pass checks. Passing CI is required to merge. Dropping support for a Python version should include removing tests for that version, because we shouldn't waste resources testing something unsupported.

And speaking of unsupported things, the githooks are contrib stuff, so it's fine not to worry about those. Doesn't look like @HumorBaby included anything Python-version-specific in those, though.

I'm pretty sure it's because LGTM is configured with Py2.7.

It's probably because we still have Python 2.7 in CI and/or requirements files. LGTM assumes Python 3 if it doesn't see any versions specified; see https://lgtm.com/help/lgtm/python-extraction#python_setup-step

@Exirel
Copy link
Contributor Author

Exirel commented May 8, 2021

Yup yup, I agree with that. Which makes me realized that I should have opened that as a DRAFT and not a ready PR, my mistake!

My reasoning about CI stuff was to give you the code first, and see what needed to be done exactly for the CI—something I know too little about at the moment—and to do that with you. So yeah, absolutely, the CI need to be modified.

My guess is that you're probably too busy to look at that now, and I'm sure it's better to wait after 7.1 is released than right now.

@dgw dgw marked this pull request as draft May 9, 2021 05:37
@dgw
Copy link
Member

dgw commented May 9, 2021

Dropped back to draft status. You're correct, I'd rather spend my time on 7.1 stuff now, and tackle this after it's released. 😸

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Surprisingly few line notes, considering the scope of this PR. Given that it's mostly deleting dead code and swapping imports, though, it's actually not that surprising after all.

Hopefully LGTM will figure out that we don't care about py2 any more now that CI and everything has been reworked to target only 3.6+, and stop giving irrelevant warnings.

checkstyle.sh Show resolved Hide resolved
pytest_run.py Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
sopel/__init__.py Outdated Show resolved Hide resolved
sopel/tools/__init__.py Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
sopel/config/__init__.py Outdated Show resolved Hide resolved
sopel/modules/ip.py Outdated Show resolved Hide resolved
sopel/test_tools.py Outdated Show resolved Hide resolved
sopel/tools/web.py Show resolved Hide resolved
@Exirel Exirel force-pushed the remove-python27 branch from 2e66a61 to dc1aedd Compare June 5, 2021 18:14
@lgtm-com

This comment has been minimized.

@lgtm-com
Copy link

lgtm-com bot commented Jun 5, 2021

This pull request introduces 2 alerts and fixes 1 when merging cfba6d7 into 56d84d1 - view on LGTM.com

new alerts:

  • 1 for 'input' function used in Python 2
  • 1 for Result of integer division may be truncated

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@Exirel Exirel marked this pull request as ready for review June 5, 2021 20:15
sopel.py Outdated Show resolved Hide resolved
pytest_run.py Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@dgw
Copy link
Member

dgw commented Jun 6, 2021

I'm baffled after studying LGTM's docs about Python version selection. It should default to Python 3, but isn't. Let's ignore the new alerts on this PR and look into it separately if they still appear on master after merging.

@half-duplex
Copy link
Member

half-duplex commented Jun 8, 2021

Depending on the scope of this PR...

  • requirements.txt and dev-requirements.txt should have the python_version == '2.7' etc lines removed. This also "fixes" CVE-2021-33503 (DoS), which sopel 7 will remain vulnerable to.
  • docs/source/configuration.rst has a comment about py2.7
  • README.rst says "Sopel requires Python 2.7.x or Python 3.3+ to run", etc
  • The shebang of sopel/cli/run.py specifically uses 2.7 (dear god, why? does this even need a shebang?)

To be done in future PR:

  • sopel/formatting.py TODO
  • conftest.py 2x "Python 3.3" comments
  • sopel/plugins/rules.py has a 2.7 TODO
  • sopel/irc/backends.py SSLAsynchatBackend.handle_connect

@Exirel Exirel force-pushed the remove-python27 branch from cfba6d7 to ba971e1 Compare June 14, 2021 07:19
@Exirel
Copy link
Contributor Author

Exirel commented Jun 14, 2021

Depending on the scope of this PR...

Indeed, more work to do! I've done what I could or think is important right now.

I left aside the TODOs in sopel.plugins (there are many!), as they will be dealt with in a future PR to rework/replace what was kept for older versions of Python.

@Exirel Exirel force-pushed the remove-python27 branch from ea4a788 to 1567a7e Compare June 17, 2021 21:35
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Some more stuff, mostly about requirements. Very close!

dev-requirements.txt Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
sopel.py Outdated Show resolved Hide resolved
@dgw dgw mentioned this pull request Jun 26, 2021
4 tasks
@Exirel
Copy link
Contributor Author

Exirel commented Jun 26, 2021

Here we go. I haven't rebased/squashed yet. Other than that, it's ready for final review: everything new should go in new PRs, I think.

@dgw dgw added the Breaking Change Stuff that probably should be mentioned in a migration guide label Jun 26, 2021
@Exirel Exirel requested a review from dgw June 26, 2021 20:45
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Squash the fixups and let's ship it. "Don't sweat the little stuff," etc., there's plenty of time to catch the stragglers and handle the various TODOs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Stuff that probably should be mentioned in a migration guide High Priority Tweak
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python2 Deprecation
3 participants