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 #864

Closed
wants to merge 0 commits into from
Closed

Drop six support, drop Py35 support #864

wants to merge 0 commits into from

Conversation

dgilman
Copy link
Contributor

@dgilman dgilman commented Dec 15, 2020

Drop support for six in the codebase.

Add CI integration to test environments guaranteed to have and not have six in them.

While making all of this work, I found that a dependency of pytest was pulling in six on python 3.5. It appears the latest pytest, only released a few days ago, dropped support for python before 3.6. I didn't set out to deprecate 3.5 support here but I figure if you can't run unit tests on a platform you can't really support it. So I went ahead and bumped the minimum Python version for astroid to 3.6.

If this PR gets accepted it would be good to do a release of astroid, as it would be good to get this fix out there but I also see you have a few other interesting bug fixes in source control like the infamous exception/stacktrace memory leak.

Steps

TODO: changelog, once the PR passes review

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • Write a good description on what the PR does.

Description

Type of Changes

Type
🔨 Refactoring

Related Issue

Closes #863

@hippo91 hippo91 self-assigned this Dec 24, 2020
Copy link
Contributor

@hippo91 hippo91 left a comment

Choose a reason for hiding this comment

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

@dgilman thanks for this PR!
It is all clear for me except a probably stupid question : why should we keep tests and test environments dealing with six?
By the way , do not forget to add an entry in the ChangeLog.
Thanks again

tests/unittest_brain.py Outdated Show resolved Hide resolved
@dgilman
Copy link
Contributor Author

dgilman commented Dec 31, 2020

There is a brain file for six and it has unit tests so it makes sense to keep testing it in CI. Likewise it makes sense for a astroid developer to install six to their local venv for when they run unit tests. The thinking behind the CI work to run tests where six is guaranteed to not be installed is that this guards against someone who codes in astroid against six when it is no longer a requirement of the package. That breakage is not going to show up in their local running of tests, nor will it show up when CI is run on GitHub, but will blow up in the user's face when they try to use the released package, which would be unfortunate.

But maybe the risk of this happening is too small to care about. I totally understand if you want to boot them, let me know and I will fix up the PR.

@hippo91
Copy link
Contributor

hippo91 commented Dec 31, 2020

@dgilman to my mind we can drop all what deal with six but i would have @PCManticore opinion before doing this.

@PCManticore
Copy link
Contributor

PCManticore commented Dec 31, 2020

We still need six for Python 2/3 compatibility checker (--py3k) though, but we don't need (as far as I know) the six dependency itself.

Copy link
Contributor

@hippo91 hippo91 left a comment

Choose a reason for hiding this comment

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

@dgilman i still have some questions. I'm sorry if some of them are a bit silly but i'm not yet very used to travis coupled to tox...

tests/unittest_brain.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tests/unittest_scoped_nodes.py Outdated Show resolved Hide resolved
@dgilman
Copy link
Contributor Author

dgilman commented Dec 31, 2020

I was not familiar with the --py3k option to pylint, and in fact I didn't attempt to run this PR's version of astroid with pylint at all. I will try running some pylint testing this weekend.

@hippo91
Copy link
Contributor

hippo91 commented Jan 2, 2021

@dgilman IMHO this PR can be merged into master. However i would have @PCManticore opinion because we maybe want to release an astroid version with python 3.9 support + python 3.5 deprecation warning before definitely throwing away python 3.5 support.

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.

The cleanup of all the @test_utils.require_version is pretty nice !

@hippo91 hippo91 mentioned this pull request Jan 23, 2021
2 tasks
@AWhetter
Copy link
Contributor

Given that Python 3.5 is end of life, I'm happy for us to drop support for that in the next version without releasing a version with deprecation warnings first. We're correctly setting python_requires so it won't break things for people. However we should sync this up with dropping support in pylint as well. Thankfully the lovely @dgilman has got a pull request in for that too.

@PCManticore
Copy link
Contributor

I concur with Ashley. We should still keep the six transform in astroid but otherwise, feel free to drop Python 3.5 and six code paths from the code base.

@hippo91 hippo91 closed this Feb 7, 2021
@hippo91
Copy link
Contributor

hippo91 commented Feb 7, 2021

@dgilman i'm sorry i did a wrong manipulation when trying to fix merge problems. I think i broke this PR.
Hopefully i got a copy on my local machine and will make a new PR.
Sorry for that.

@hippo91 hippo91 mentioned this pull request Feb 7, 2021
@hippo91
Copy link
Contributor

hippo91 commented Feb 7, 2021

See PR #889

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please bump dependency version of six
5 participants