[BEAM-3738] Add more flake8 tests to run_pylint.sh #4798

cclauss · 2018-03-05T16:17:04Z

My sense is that E901,E999,F821,F822,F823 are the "showstopper" flake8 issues that can halt the runtime with a SyntaxError, NameError, etc. The other flake8 issues are merely "style violations" -- useful for readability but they do not effect runtime safety.

This PR therefore recommends replacing --select=E999 with --select=E901,E999,F822,F823 because the codebase currently passes all of these tests and we would like to avoid any backsliding. We would also want to add F821 (undefined names!) to that list but work must be completed on basestring, buffer(), cmp(), file(), reduce(), unicode, xrange(), and #4561 before that can be done.

F821: undefined name name
F822: undefined name name in __all__
F823: local variable name referenced before assignment
E901: SyntaxError or IndentationError
E999: SyntaxError -- failed to compile a file into an Abstract Syntax Tree

DESCRIPTION HERE

Follow this checklist to help us incorporate your contribution quickly and easily:

aaltay · 2018-03-06T19:00:59Z

Could you also update https://github.com/apache/beam/blob/master/sdks/python/run_mini_py3lint.sh ?

cclauss · 2018-03-06T20:22:19Z

~~Merge #4807 or similar before merging this PR.~~

Once #4561 and ~~#4806~~ (or similar) are merged then python2 -m flake8 . --select=F821 should pass but python3 -m flake8 . --select=F821 will require additional work.

robertwb · 2018-03-06T20:44:03Z

+1 to this direction.

On a higher note, is there a JIRA issue or thread we should be coordinating this work on (or at least referencing).

cclauss · 2018-03-06T20:48:36Z

In general, I have been focused on https://issues.apache.org/jira/browse/BEAM-1251

aaltay · 2018-03-06T22:00:38Z

@cclauss https://issues.apache.org/jira/browse/BEAM-1251 is an umbrella bug for all python 3 changes. Whenever possible, it makes sense to create sub-issues. Lint for python 3 support should have its own issue at least.

Could you also reference issues in the PR title in the "[BEAM-XYZ] ..." format?

cclauss · 2018-03-23T18:43:14Z

Now that #4877 is merged, can we get another review on these changes to avoid backslides? @udim

udim · 2018-03-23T18:50:48Z

retest this please

aaltay · 2018-03-27T17:30:39Z

sdks/python/run_mini_py3lint.sh

@@ -37,4 +37,5 @@ if test $# -gt 0; then
 fi

 echo "Running flake8 for module $MODULE:"
-flake8 $MODULE --count --select=E999 --show-source --statistics
+# TODO: Add F821 (undefined names) as soon as that test passes


Which test passes? Do we have a JIRA tracking this?

Also why F821 is special compared to any other warning to be mentioned in this todo?

All showstopper issue tests pass except for F821 which is what makes it special.

There are other errors/violation codes that are not enabled including F821: http://flake8.pycqa.org/en/latest/user/error-codes.html

Please reread the first sentence of description above which discusses the "showstopper" flake8 issues vs. the "style violations". I have been focused on the former for the reasons described above.

It is fine to move that sentence to a JIRA, and add a todo referencing that JIRA. People reading that TODO once it is merged would not easily be able to find the PR and understand what is the todo for, and what needs to happen to resolve it.

You can also reference the issues you have noticed with F821 as you mentioned in your other comment in that same JIRA.

cclauss · 2018-03-27T19:41:42Z

F821 status (there are PRs open for most of these)...

$ python3 -m flake8 . --count --select=E9,F821,F822,F823 --show-source --statistics

./sdks/python/apache_beam/io/vcfio_test.py:77:14: F821 undefined name 'cmp'
      return cmp(v1.end, v2.end)
             ^
./sdks/python/apache_beam/io/vcfio_test.py:78:12: F821 undefined name 'cmp'
    return cmp(v1.start, v2.start)
           ^
./sdks/python/apache_beam/io/vcfio_test.py:79:10: F821 undefined name 'cmp'
  return cmp(v1.reference_name, v2.reference_name)
         ^
./sdks/python/apache_beam/io/avroio.py:313:34: F821 undefined name 'buffer'
      result = snappy.decompress(buffer(data)[:-4])
                                 ^
./sdks/python/apache_beam/io/gcp/datastore/v1/helper.py:93:12: F821 undefined name 'cmp'
  result = cmp(p1.kind, p2.kind)
           ^
./sdks/python/apache_beam/io/gcp/datastore/v1/helper.py:101:12: F821 undefined name 'cmp'
    return cmp(p1.id, p2.id)
           ^
./sdks/python/apache_beam/io/gcp/datastore/v1/helper.py:106:10: F821 undefined name 'cmp'
  return cmp(p1.name, p2.name)
         ^
./sdks/python/apache_beam/utils/windowed_value.py:196:14: F821 undefined name 'cmp'
      return cmp(type(left), type(right))
             ^
./sdks/python/apache_beam/utils/timestamp.py:100:12: F821 undefined name 'cmp'
    return cmp(self.micros, other.micros)
           ^
./sdks/python/apache_beam/utils/timestamp.py:178:12: F821 undefined name 'cmp'
    return cmp(self.micros, other.micros)
           ^
./sdks/python/apache_beam/testing/test_stream.py:51:14: F821 undefined name 'cmp'
      return cmp(type(self), type(other))
             ^
./sdks/python/apache_beam/testing/test_stream.py:66:12: F821 undefined name 'cmp'
    return cmp(self.timestamped_values, other.timestamped_values)
           ^
./sdks/python/apache_beam/testing/test_stream.py:76:12: F821 undefined name 'cmp'
    return cmp(self.new_watermark, other.new_watermark)
           ^
./sdks/python/apache_beam/testing/test_stream.py:86:12: F821 undefined name 'cmp'
    return cmp(self.advance_by, other.advance_by)
           ^
./sdks/python/apache_beam/transforms/window.py:195:12: F821 undefined name 'cmp'
    return cmp(self.end, other.end) or cmp(hash(self), hash(other))
           ^
./sdks/python/apache_beam/transforms/window.py:195:40: F821 undefined name 'cmp'
    return cmp(self.end, other.end) or cmp(hash(self), hash(other))
                                       ^
./sdks/python/apache_beam/transforms/window.py:250:14: F821 undefined name 'cmp'
      return cmp(type(self), type(other))
             ^
./sdks/python/apache_beam/transforms/window.py:251:12: F821 undefined name 'cmp'
    return cmp((self.value, self.timestamp), (other.value, other.timestamp))
           ^
./sdks/python/apache_beam/runners/portability/fn_api_runner_test.py:103:11: F821 undefined name 'MetricKey'
          MetricKey('mydofn',
          ^
./sdks/python/apache_beam/runners/portability/fn_api_runner_test.py:104:21: F821 undefined name 'MetricName'
                    MetricName('ns1', 'elements')),
                    ^
./sdks/python/apache_beam/runners/portability/fn_api_runner_test.py:105:11: F821 undefined name 'MetricKey'
          MetricKey('myotherdofn',
          ^
./sdks/python/apache_beam/runners/portability/fn_api_runner_test.py:106:21: F821 undefined name 'MetricName'
                    MetricName('ns2', 'elementsplusone'))])
                    ^
./sdks/python/apache_beam/runners/worker/statesampler_slow.py:54:12: F821 undefined name 'StateSamplerInfo'
    return StateSamplerInfo(
           ^
23    F821 undefined name 'buffer'
23

cclauss · 2018-03-28T13:30:31Z

https://issues.apache.org/jira/browse/BEAM-3959

aaltay

Thank you. I will update the the todo to mention the newly opened JIRA during merge.

cclauss mentioned this pull request Mar 6, 2018

[BEAM-1251] Change unicode --> six.text_type for Python 3 #4697

Merged

udim mentioned this pull request Mar 6, 2018

Add missing import statements. #4806

Merged

cclauss force-pushed the patch-1 branch from 1862fed to dc8bcb6 Compare March 6, 2018 20:06

cclauss changed the title ~~Update run_pylint.sh~~ [BEAM-3738] Add more flake8 tests to run_pylint.sh Mar 6, 2018

cclauss force-pushed the patch-1 branch from dc8bcb6 to b7dd2fe Compare March 6, 2018 23:51

cclauss force-pushed the patch-1 branch from b7dd2fe to 37432ac Compare March 17, 2018 09:04

[BEAM-3738] Add more flake8 tests to run_pylint.sh

8cf178d

cclauss force-pushed the patch-1 branch from 37432ac to 8cf178d Compare March 17, 2018 09:05

cclauss mentioned this pull request Mar 17, 2018

[BEAM-1251] Upgrade from buffer to memoryview for Python 3 #4820

Closed

10 tasks

udim approved these changes Mar 26, 2018

View reviewed changes

aaltay reviewed Mar 27, 2018

View reviewed changes

aaltay approved these changes Mar 28, 2018

View reviewed changes

asfgit closed this in 6b357c6 Mar 28, 2018

cclauss deleted the patch-1 branch March 28, 2018 20:25

cclauss mentioned this pull request Apr 3, 2018

[BEAM-3731] Enable tests to run in Python 3 #4730

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-3738] Add more flake8 tests to run_pylint.sh #4798

[BEAM-3738] Add more flake8 tests to run_pylint.sh #4798

cclauss commented Mar 5, 2018 •

edited

Loading

aaltay commented Mar 6, 2018

cclauss commented Mar 6, 2018 •

edited

Loading

robertwb commented Mar 6, 2018

cclauss commented Mar 6, 2018 •

edited

Loading

aaltay commented Mar 6, 2018

cclauss commented Mar 23, 2018

udim commented Mar 23, 2018

aaltay Mar 27, 2018

cclauss Mar 27, 2018 •

edited

Loading

aaltay Mar 27, 2018

cclauss Mar 27, 2018

aaltay Mar 27, 2018

cclauss commented Mar 27, 2018

cclauss commented Mar 28, 2018

aaltay left a comment

[BEAM-3738] Add more flake8 tests to run_pylint.sh #4798

[BEAM-3738] Add more flake8 tests to run_pylint.sh #4798

Conversation

cclauss commented Mar 5, 2018 • edited Loading

aaltay commented Mar 6, 2018

cclauss commented Mar 6, 2018 • edited Loading

robertwb commented Mar 6, 2018

cclauss commented Mar 6, 2018 • edited Loading

aaltay commented Mar 6, 2018

cclauss commented Mar 23, 2018

udim commented Mar 23, 2018

aaltay Mar 27, 2018

Choose a reason for hiding this comment

cclauss Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

aaltay Mar 27, 2018

Choose a reason for hiding this comment

cclauss Mar 27, 2018

Choose a reason for hiding this comment

aaltay Mar 27, 2018

Choose a reason for hiding this comment

cclauss commented Mar 27, 2018

cclauss commented Mar 28, 2018

aaltay left a comment

Choose a reason for hiding this comment

cclauss commented Mar 5, 2018 •

edited

Loading

cclauss commented Mar 6, 2018 •

edited

Loading

cclauss commented Mar 6, 2018 •

edited

Loading

cclauss Mar 27, 2018 •

edited

Loading