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

Drop six support, drop Py35 support #4006

Closed
wants to merge 7 commits into from
Closed

Drop six support, drop Py35 support #4006

wants to merge 7 commits into from

Conversation

dgilman
Copy link
Contributor

@dgilman dgilman commented Jan 1, 2021

This PR is the pylint sibling to pylint-dev/astroid#864 . It updates the pylint test suite to run successfully in environments where six is not installed. The build is expected to fail until that PR is merged in.

Note that there are still a bunch of tests that reference the six module. These tests still pass because pylint didn't have to import six in order to do its linting.

Because the astroid PR bumps the minimum required Python to 3.6 I have also added commits to drop Python 3.5 support here. The logic from the other PR: pytest recently updated to require Python 3.6+. Although there are older versions that support older Pythons those versions have a dependency on six, making it impossible to assert in CI that six is not installed and that pylint/astroid runs correctly when six is not installed. If you can't run pytest on a platform you can't really support it. Because Python 3.5 is at a hard EOL and completely unmaintained it's about time to drop Python 3.5 support anyway.

The removed tests are ones that were always skipped because they could only run under Pythons before 3.6. If pylint is run under a Python 3.x interpreter against a Python 2.x file they result in syntax errors instead of the relevant warnings/errors. Because pylint itself has run under Python 3.x only for a few releases now I think it's safe to drop these unit tests as the situations they test against can no longer be reached.

I did some investigation into the --py3k option per PCManticore's comments in the other PR. The feature appears to not be implemented using pip (anymore?) so there was nothing to change. While doing this investigation I found a unit test that was accidentally deleted but is still relevant for --py3k users and brought it back in.

The CHANGELOG and such are referencing a pylint 2.6.1 release but it seems like a change in minimum required Python version justifies a bump to pylint 2.7.0.

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

Type of Changes

Type
🔨 Refactoring

Related Issue

@Pierre-Sassoulas Pierre-Sassoulas linked an issue Jan 3, 2021 that may be closed by this pull request
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.7.0 milestone Jan 3, 2021
@hippo91 hippo91 self-assigned this Feb 7, 2021
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Nice cleanup, will merge when the test are fixed on master.

@dgilman
Copy link
Contributor Author

dgilman commented Feb 11, 2021

This PR is supposed to help with some of the broken tests after the sister PR was merged to astroid. There are still some other tests relating to spurious "unused-import" with the few tests that still use six (and only as a base class), maybe that is the issue at hand?

At any rate there were some merge conflicts so I've rebased on latest master.

Comment on lines +1 to +2
[testoptions]
requires = six
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not doing as in unpacking_non_sequence.py and remove six import ?

Comment on lines +1 to +2
[testoptions]
requires = six
Copy link
Contributor

Choose a reason for hiding this comment

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

Idem

Comment on lines +1 to +2
[testoptions]
requires = six
Copy link
Contributor

Choose a reason for hiding this comment

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

Idem.
In fact those tests are those which are failing on the CI after pylint-dev/astroid#841 has been merged...

@hippo91
Copy link
Contributor

hippo91 commented Feb 11, 2021

@dgilman i'm sorry but i do not understand what is the criteria to remove or keep six import in unit test files. There are still a few unit test that import six and others that do not have this import anymore...

At this point there are still a handful of packages that are pulling in
six so it is not easy to get a six-free testing environment. Maybe in a
few months or a year the community will start its detachment from six
and this can come back in.
@dgilman
Copy link
Contributor Author

dgilman commented Feb 11, 2021

They're kept in there because astroid has special handling for six.with_metaclass, as documented here. astroid and pylint tests which touched with_metaclass are kept using six because you need it to exercise that special handling.

I can't comment on the correctness or not of #841's handling of with_metaclass, but it does seem suspicious that the PR does not acknowledge the existing support or modify it in any way.

I am going to push a change here that drops the TestNoSix class. It seems like there are still a few dependencies, especially on the Windows builders, that are pulling in six so it may not be possible to test in a six-less environment.

@dgilman
Copy link
Contributor Author

dgilman commented Feb 11, 2021

Also, to clarify a bit more about six vs not six: if you look at unpacking_non_sequence.py it is using a plain with_metaclass(MetaIter) in the six version. The metaclass=MetaIter is equivalent.

The other tests have six.with_metaclass(MetaSubscriptable, object). When I was going through the testing (which was a bit ago, I forget some of the details here) the two argument form of with_metaclass wasn't equivalent to a plain metaclass=foo and could only be emulated with six.

@Pierre-Sassoulas
Copy link
Member

Closed in favor of #4091

@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.8.0, 2.7.3 Mar 28, 2021
@Pierre-Sassoulas Pierre-Sassoulas added Maintenance Discussion or action around maintaining pylint or the dev workflow and removed task labels Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop support of Python 3.5
3 participants