[BEAM-2821] Run isort and autopep8 in preparation for futurize.#3785
[BEAM-2821] Run isort and autopep8 in preparation for futurize.#3785holdenk wants to merge 17 commits into
Conversation
holdenk
changed the title
|
Run Python Postcommit |
holdenk
changed the title
|
cc @robertwb |
robertwb
left a comment
robertwb
left a comment
There was a problem hiding this comment.
Is there an option to put multiple imports on distinct lines. For example
from apache_beam.transforms import core, ptransform, window
is particularly odd. Also, we prefer long lines to wrapping with \.
| import logging | ||
| import unittest | ||
|
|
||
|
|
There was a problem hiding this comment.
Don't remove this comment.
| from apache_beam.io.concat_source import ConcatSource | ||
| from apache_beam.io.filebasedsource import _SingleFileSource as SingleFileSource | ||
|
|
||
| from apache_beam.io.filebasedsource import \ |
There was a problem hiding this comment.
default isort was 79, switched to 80 and reverted
|
@robertwb there is an option to do multi imports per single line with isort, let me poke at it. |
|
An unexpected benefit of SL mode is it seems to agree with pylint a lot more (which is good since newer pylints use isort internally so the disagreements are a little confusing). |
|
The flink runner failed which doesn't make a lot of sense for the changes, jenkins retest this please. |
holdenk
force-pushed
the
BEAM-2821-isort-and-autopep8
branch
from
f4bf855 to
7ee5d4e
Compare
robertwb
left a comment
robertwb
left a comment
There was a problem hiding this comment.
Thanks! Basically looks good to me. Just a couple of comments, and also there's a few more files that need to be fixed/ignored according to jenkins:
ERROR: /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/python/apache_beam/io/gcp/datastore/v1/datastoreio.py Imports are incorrectly sorted.
ERROR: /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/python/apache_beam/io/gcp/datastore/v1/adaptive_throttler_test.py Imports are incorrectly sorted.
ERROR: /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/python/apache_beam/runners/direct/transform_evaluator.py Imports are incorrectly sorted.
ERROR: /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall/sdks/python/apache_beam/runners/dataflow/native_io/iobase_test.py Imports are incorrectly sorted.
| from apache_beam.transforms.display import DisplayData | ||
| from apache_beam.transforms.display_test import DisplayDataItemMatcher | ||
|
|
||
| # Importing following private class for testing purposes. |
There was a problem hiding this comment.
Not a huge fan of this change, but it's probably not worth fighting the tool...
| from apitools.base.py import base_api | ||
|
|
||
| from apache_beam.io.gcp.internal.clients.bigquery import bigquery_v2_messages as messages | ||
| from apache_beam.io.gcp.internal.clients.bigquery import \ |
There was a problem hiding this comment.
I prefer ignoring line length limits for these imports over wrapping.
| # Skip files where isort is behaving weirdly | ||
| ISORT_EXCLUDED=( | ||
| "avroio_test.py" | ||
| "fast_coders_test.py" |
There was a problem hiding this comment.
This is an odd ordering. At least put the two coders tests next to each other.
…dering a bit, manually remove single line isort continuations as asked to remove.
|
Thanks @robertwb ! I've updated it a bit, I hope your staying cool in the heat wave (if you're in an area impacted by it). Looking forward to moving on to the next bit of Py3 support :) |
|
LGTM |
|
Thanks 😀 |
3 participants
In preparation for running futurize (py2/3 support) run isort on the imports and autopep8 ( W391,W293,W291,E306,E305,E304,E303). This also adds isort to run_pylint but skips the files where isort was doing things which conflicted with pylint.