-
-
Notifications
You must be signed in to change notification settings - Fork 274
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
Conversation
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.
@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
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. |
@dgilman to my mind we can drop all what deal with |
We still need |
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.
@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
...
I was not familiar with the |
@dgilman IMHO this PR can be merged into master. However i would have @PCManticore opinion because we maybe want to release an |
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.
The cleanup of all the @test_utils.require_version
is pretty nice !
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 |
I concur with Ashley. We should still keep the |
@dgilman i'm sorry i did a wrong manipulation when trying to fix merge problems. I think i broke this PR. |
See PR #889 |
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
Description
Type of Changes
Related Issue
Closes #863