-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
drop python<=3.7 support #170
base: master
Are you sure you want to change the base?
Conversation
According to https://endoflife.date/python python 3.7 has been EOSed 27 Jun 2023. Filter all code over `pyupgracde --py38-plus`. Signed-off-by: Tomasz Kłoczko <[email protected]>
Signed-off-by: Tomasz Kłoczko <[email protected]>
Signed-off-by: Tomasz Kłoczko <[email protected]>
Signed-off-by: Tomasz Kłoczko <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This PR is missing a changelog update in CHANGES.rst
.
It is also missing a corresponding Python version change in tox.ini
, appveyor.yml
-- I would recommend running check-python-versions
to verify that these are all consistent, or you can use check-python-versions --drop 3.7
to automate updating all the files.
I think GitHub Actions will show the other issues.
self.assertTrue( | ||
str(cm.exception).startswith(should_start_with), | ||
'\n%r does not start with\n%r' % (str(cm.exception), | ||
'\n{!r} does not start with\n{!r}'.format(str(cm.exception), | ||
should_start_with)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is now misindented: it was matching the position of the (
in the previous line, and no longer does. (The pain of these indentation adjustments is one of the reasons I've lately switched to a more Black-inspired indentation style with a line break right after the opening (
and a 4-space increase of indentation on the next line.)
If we used an f-string here, maybe it would fit without wrapping?
from check_manifest import rmtree | ||
|
||
|
||
CAN_SKIP_TESTS = os.getenv('SKIP_NO_TESTS', '') == '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do things not break if you remove this line? Something is wrong.
@@ -1165,8 +1144,6 @@ class TestBzr(VCSMixin, unittest.TestCase): | |||
vcs = BzrHelper() | |||
|
|||
|
|||
@unittest.skipIf(HAS_OEM_CODEC, | |||
"Python 3.6 lets us use 'oem' codec instead of guessing") | |||
class TestBzrTerminalCharsetDetectionOnOldPythons(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these two test classes could possibly work at the same time. This one is meant for Python < 3.6 and should be removed entirely.
- "3.8" | ||
- "3.9" | ||
- "3.10" | ||
- "3.11" | ||
- "3.12" | ||
- "pypy-3.7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some newer PyPy version instead. I don't want to claim PyPy compatibility if I don't run tests on PyPy in CI.
# Python >= 3.6 on Windows | ||
HAS_OEM_CODEC = True | ||
|
||
from unittest import mock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import needs to be moved above the from xml.etree
import or isort will complain.
Also, I need to review my isort command-line because a complaint of the sort
ERROR: /home/runner/work/check-manifest/check-manifest/tests.py Imports are incorrectly sorted and/or formatted.
is useless when it won't tell me which imports where and how to sort them. It could show a diff or something.
# all the features used in the test suite | ||
import mock | ||
|
||
from check_manifest import rmtree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import is necessary and removing it breaks the test suite (which you can only see on Appveyor because GitHub Actions cancels all the useful test runs early after seeing the isort failure).
@@ -179,7 +158,7 @@ def test_copy_files(self): | |||
'cp a %s' % n('/dest/dir/a'), | |||
'mkdir %s' % n('/dest/dir/b'), | |||
'makedirs %s' % n('/dest/dir/c/d'), | |||
'cp %s %s' % (n('c/d/e'), n('/dest/dir/c/d/e')), | |||
'cp {} {}'.format(n('c/d/e'), n('/dest/dir/c/d/e')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, why change this line but leave all of the previous lines using %-formatting? This offends my sense of consistency.
Question: does having Python 3.7 support cause you any actual issues? (If this is just a general maintenance task because you're bored and looking for things to do, that's also fine! But I will prioritize the PR more if I know there's a problem that causes issues for someone.) |
Updating code to use latest possible syntax/API is not about solving aby problem. |
According to https://endoflife.date/python python 3.7 has been EOSed 27 Jun 2023.
Filter all code over
pyupgracde --py38-plus
.