Skip to content

Drop Python 2 Support#594

Closed
lukehinds wants to merge 12 commits into
PyCQA:masterfrom
lukehinds:burn-py2
Closed

Drop Python 2 Support#594
lukehinds wants to merge 12 commits into
PyCQA:masterfrom
lukehinds:burn-py2

Conversation

@lukehinds
Copy link
Copy Markdown
Member

Python 2 is well beyond EOL now and will soon be removed from
github's workflows

This PR seeks to remove all calls / references and tests that
rely on Python 2.

  • Remove six imports and six.Py2 conditionals
  • Remove Py2 calls from github workflows
  • Merged example files (e.g exec-py2.py|exec-py3.py > exec.py)
  • Removed py2 env from setuptools
  • Removed py2 env from tox

Resolves: #584

Python 2 is well beyond EOL now and will soon be removed from
github's workflows

This PR seeks to remove all calls / references and tests that
rely on Python 2.

* Remove six imports and six.Py2 conditionals
* Remove Py2 calls from github workflows
* Merged example files (e.g exec-py2.py|exec-py3.py > exec.py)
* Removed py2 env from setuptools
* Removed py2 env from tox

Resolves: PyCQA#584
@lukehinds
Copy link
Copy Markdown
Member Author

lukehinds commented Mar 30, 2020

hi @ericwb / @sigmavirus24

A six.Py2 section I was not sure how to handle and wanted your input:

arg_name = name.id if six.PY2 else name.arg from bandit/plugins/django_xss.py

def evaluate_var(xss_var, parent, until, ignore_nodes=None):
   secure = False
   if isinstance(xss_var, ast.Name):
       if isinstance(parent, ast.FunctionDef):
           for name in parent.args.args:
               arg_name = name.id if six.PY2 else name.arg
               if arg_name == xss_var.id:
                   return False  # Params are not secure

I have never seen a conditional following a declaration like that, but figured it might be a six way of doing things.

Looks like we can just remove if six.PY2 else name.arg:

 for name in parent.args.args:
    arg_name = name.id

@sigmavirus24
Copy link
Copy Markdown
Member

You return .id if it's python 2 otherwise use .arg so we should replace everything after the = up to, and including, the else.

This is a Python ternary, I agree they can be disorienting at times

@lukehinds
Copy link
Copy Markdown
Member Author

How's this @sigmavirus24

5262e9b

Comment thread bandit/plugins/django_xss.py Outdated
@sigmavirus24
Copy link
Copy Markdown
Member

Comment thread bandit/plugins/django_xss.py Outdated
Comment thread bandit/plugins/django_xss.py Outdated
Copy link
Copy Markdown
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

Copy link
Copy Markdown
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

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

Can you also add python_requires='>=3.5' to strict enforcing versions where it can be installed.

https://packaging.python.org/guides/dropping-older-python-versions/#specify-the-version-ranges-for-supported-python-distributions

@lukehinds
Copy link
Copy Markdown
Member Author

Should I squash and merge @ericwb , do we need an ack from @sigmavirus24 ?

@ericwb
Copy link
Copy Markdown
Member

ericwb commented Apr 1, 2020

@lukehinds Looks like there are still some things to fix up in the build.

| B321 | ftplib | - ftplib.\* | High |
+------+---------------------+------------------------------------+-----------+

B322: input
Copy link
Copy Markdown
Member Author

@lukehinds lukehinds Apr 15, 2020

Choose a reason for hiding this comment

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

@ericwb removed B322 inline with #596

Was not sure if I should leave B322 as a gap or I should count -1 on the other remain tests. So for example B323 becomes B322, B324 becomes B323 etc. I was thinking around folks who are cherry picking tests to run and if we increment down, they may end up having an incorrect mapping.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, we have to keep legacy numbers for historical reasons as to not break users including/excluding certain numbers. We have other examples where we leave the documentation for the ID, but indicate to the users that the test has been removed.

@lukehinds
Copy link
Copy Markdown
Member Author

@sigmavirus24 / @ericwb

Seeing this failure a lot, any ideas, have you come across it before:

 Captured traceback:
~~~~~~~~~~~~~~~~~~~
    Traceback (most recent call last):

      File "/home/runner/work/bandit/bandit/tests/functional/test_baseline.py", line 248, in test_new_candidates_include_nosec_only_nosecs
    self.assertIn(new_candidates_some_total_lines, return_value)

      File "/home/runner/work/bandit/bandit/.tox/py35/lib/python3.5/site-packages/testtools/testcase.py", line 421, in assertIn
    self.assertThat(haystack, Contains(needle), message)

      File "/home/runner/work/bandit/bandit/.tox/py35/lib/python3.5/site-packages/testtools/testcase.py", line 502, in assertThat
    raise mismatch_error

    testtools.matchers._impl.MismatchError: 'Total lines of code: 9' not in ''

@sigmavirus24
Copy link
Copy Markdown
Member

I haven't come across it before but I'm unsurprised that checking the output seems to be causing us grief here.

I wonder if we can change our helper function to give us more detail here. I wonder if we could use check_output instead of subprocess.Popen.

Copy link
Copy Markdown
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

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

Rather than trying to drop all of Py2 references in one PR, might be better to do incremental changes starting with the dropping of build of 2.7 and installability of 2.7.

@ericwb
Copy link
Copy Markdown
Member

ericwb commented May 18, 2020

@lukehinds I pushed a commit to do the bare minimum to remove Py2.7. Please rebase and resolve conflicts.

@lukehinds
Copy link
Copy Markdown
Member Author

thanks @ericwb , I am still working through this, but its a huge list of stuff breaking - quite surprised at how much this has broken.

@ericwb ericwb requested a review from ghugo as a code owner December 20, 2020 07:28
@ericwb
Copy link
Copy Markdown
Member

ericwb commented Dec 20, 2020

Think this can be closed, as changes were made in various other PRs, namely: #674, #662, #615, etc

@ericwb ericwb closed this Dec 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deprecate Python 2

3 participants