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 Python 2 Support #594

Closed
wants to merge 12 commits into from
Closed

Drop Python 2 Support #594

wants to merge 12 commits into from

Conversation

lukehinds
Copy link
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
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
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
Member Author

How's this @sigmavirus24

5262e9b

@sigmavirus24
Copy link
Member

Copy link
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
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
Member Author

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

@ericwb
Copy link
Member

ericwb commented Apr 1, 2020

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

@@ -265,20 +265,6 @@
| B321 | ftplib | - ftplib.\* | High |
+------+---------------------+------------------------------------+-----------+

B322: input
Copy link
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
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
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
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
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
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
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
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