Fix issues with Python 3.6.0 #4446

pkolbus · 2021-05-06T15:26:40Z

Steps

Add yourself to CONTRIBUTORS if you are a new contributor.
Add a ChangeLog entry describing what your PR does.
If it's a new feature or an important bug fix, add a What's New entry in
doc/whatsnew/<current release.rst>.
Write a good description on what the PR does.

Description

This PR fixes compatibility with Python 3.6.0, mainly due to typing:

typing.Counter is added in Python 3.6.1, so make the import conditional on TYPE_CHECKING.
typing.NoReturn is added in Python 3.6.2, split the inconsistent_returns tests that need this to it's own file.
black, pre-commit and pyupgrade require Python 3.6.1 (or later), add python_full_version markers so tox -e py36 works

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

Closes #4412

Pierre-Sassoulas

Thank you this look dandy 😄 !

It seems like the tests for python 3.8 3.9 are flaky. We also encountered the problem in #4442, so definitely not a problem with what you did.

pkolbus · 2021-05-06T15:43:50Z

Thanks!

pylint/checkers/strings.py

cdce8p · 2021-05-07T15:29:45Z

It seems like the tests for python 3.8 3.9 are flaky. We also encountered the problem in #4442, so definitely not a problem with what you did.

The tests are now fixed. A rebase should do it

coveralls · 2021-05-09T14:27:39Z

Coverage decreased (-0.006%) to 91.721% when pulling d6e5e89 on pkolbus:issue-4412-python-360 into 9b268ec on PyCQA:master.

cdce8p

I pushed a commit with some minor style changes directly to your branch. It seems though, that there is a merge conflict with ChangeLog.

cdce8p · 2021-05-10T22:17:17Z

tox.ini

-    -r {toxinidir}/requirements_test.txt
+    -r {toxinidir}/requirements_test_min.txt


I don't really like this change. requirements_test.txt also includes optional dependencies for pytest that are not tested without them. Does tox fail completely if it can't install all dependencies, even if only python_requires doesn't match?

If that is the case, I would like to suggest to remove py36 from the envlist. It will still be tested through Github actions.

Hmm, good point. Yes, tox fails completely. Removing py36 from the envlist doesn’t really solve the goal either, which is to test that version locally, and would just make things harder on other contributors running with 3.6.2+.

I’ll take another look at these and ponder on it a bit. Maybe move the pytest extensions to _min or otherwise refactor the way things are broken down? Or add python_requires markers where needed (although that feels a bit like polluting the code). Or just drop this part of the change, especially if it’s not worth adding CI coverage for 3.6.0?

python_requires would work. As I understand it, we just need to make sure black isn't installed for 3.6.0, am I right?

Thanks, will work on the marker way. pre-commit is 3.6.2+ too, might be one or two others too. Will find them in testing 😀

Just though about something else. This might work even better: https://tox.readthedocs.io/en/latest/config.html#generating-environments-conditional-settings

Sorry, missed until now. The trick with the conditional factors is that they have to appear in the env name, it couldn't be automatic based on the python_full_version. Closest we'd get would be something like tox -e py36-min that installed requirements_test_min.txt instead of requirements_test.txt. The python_full_version markers would let the pytest extensions run on 3.6.0 if we needed them to.

Turns out there were only three packages needing markers... let me know what you think. Happy to keep iterating on this.

pkolbus · 2021-05-10T23:33:09Z

I pushed a commit with some minor style changes directly to your branch. It seems though, that there is a merge conflict with ChangeLog.

Thanks for the heads up. The changes look good to me, will keep them when I rebase again.

The requirements_test.txt includes black, pre-commit, and pyupgrade which are not compatible with Python 3.6.0. Add python_full_version markers so that `tox -e py36` works with 3.6.0.

typing.Counter was added in Python 3.6.1. In the strings checker, guard the import with TYPE_CHECKING and use a type annotation comment to fix usage with Python 3.6.0.

typing.NoReturn was introduced in Python 3.6.2. Move the tests for issue pylint-dev#4122 to a separate file and skip when Python is too old. Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

cdce8p · 2021-05-12T10:58:18Z

requirements_test.txt

@@ -2,7 +2,7 @@
 -r requirements_test_min.txt
 coveralls~=3.0
 coverage~=5.5
-pre-commit~=2.12
+pre-commit~=2.12;python_full_version>="3.6.2"


Why not use just python_version? Same in requirements_test_pre_commit.txt.
~~https://www.python.org/dev/peps/pep-0496/#version-numbers~~

platform.python_version() is a dotted string, so according to that PEP python_version doesn’t include the micro part.

Makes sense, didn't know that

pkolbus · 2021-05-12T11:46:11Z

Thanks!

cdce8p · 2021-05-12T11:57:00Z

Thanks!

@pkolbus Thank you for doing the work!

Pierre-Sassoulas approved these changes May 6, 2021

View reviewed changes

cdce8p reviewed May 7, 2021

View reviewed changes

pylint/checkers/strings.py Show resolved Hide resolved

pkolbus force-pushed the issue-4412-python-360 branch from 2465d32 to b731c73 Compare May 9, 2021 14:22

cdce8p requested changes May 10, 2021

View reviewed changes

Pierre-Sassoulas added Blocker 🙅 Blocks the next release Crash 💥 A bug that makes pylint crash labels May 11, 2021

pkolbus and others added 4 commits May 11, 2021 19:11

tox: Enable testing python 3.6.0

744ade6

The requirements_test.txt includes black, pre-commit, and pyupgrade which are not compatible with Python 3.6.0. Add python_full_version markers so that `tox -e py36` works with 3.6.0.

Fix typing.Counter import for Python 3.6.0

419c91f

typing.Counter was added in Python 3.6.1. In the strings checker, guard the import with TYPE_CHECKING and use a type annotation comment to fix usage with Python 3.6.0.

Skip typing.NoReturn tests on Python < 3.6.2

644b3ba

typing.NoReturn was introduced in Python 3.6.2. Move the tests for issue pylint-dev#4122 to a separate file and skip when Python is too old. Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

Update ChangeLog and whatsnew

d6e5e89

pkolbus force-pushed the issue-4412-python-360 branch from 5fed063 to d6e5e89 Compare May 12, 2021 00:32

pkolbus requested a review from cdce8p May 12, 2021 00:33

cdce8p requested changes May 12, 2021

View reviewed changes

cdce8p approved these changes May 12, 2021

View reviewed changes

cdce8p merged commit 318f1af into pylint-dev:master May 12, 2021

pkolbus deleted the issue-4412-python-360 branch May 12, 2021 11:45

cdce8p mentioned this pull request Sep 23, 2021

Start requiring Python 3.6.2 or newer #5065

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with Python 3.6.0 #4446

Fix issues with Python 3.6.0 #4446

pkolbus commented May 6, 2021 •

edited

Pierre-Sassoulas left a comment

pkolbus commented May 6, 2021

cdce8p commented May 7, 2021

coveralls commented May 9, 2021 •

edited

cdce8p left a comment

cdce8p May 10, 2021

pkolbus May 10, 2021

cdce8p May 10, 2021

pkolbus May 10, 2021

cdce8p May 11, 2021

pkolbus May 12, 2021

pkolbus commented May 10, 2021

cdce8p May 12, 2021 •

edited

pkolbus May 12, 2021

cdce8p May 12, 2021

pkolbus commented May 12, 2021

cdce8p commented May 12, 2021

		-r {toxinidir}/requirements_test.txt
		-r {toxinidir}/requirements_test_min.txt

Fix issues with Python 3.6.0 #4446

Fix issues with Python 3.6.0 #4446

Conversation

pkolbus commented May 6, 2021 • edited

Steps

Description

Type of Changes

Related Issue

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

pkolbus commented May 6, 2021

cdce8p commented May 7, 2021

coveralls commented May 9, 2021 • edited

cdce8p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkolbus commented May 10, 2021

cdce8p May 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkolbus commented May 12, 2021

cdce8p commented May 12, 2021

pkolbus commented May 6, 2021 •

edited

coveralls commented May 9, 2021 •

edited

cdce8p May 12, 2021 •

edited