[SPARK-23174][BUILD][PYTHON] python code style checker update #20338

rekhajoshm · 2018-01-21T03:47:47Z

What changes were proposed in this pull request?

Referencing latest python code style checking from PyPi/pycodestyle
Removed pending TODO
For now, in tox.ini excluded the additional style error discovered on existing python due to latest style checker (will fallback on review comment to finalize exclusion or fix py)
Any further code styling requirement needs to be part of pycodestyle, not in SPARK.

How was this patch tested?

./dev/run-tests

Pulling functionality from apache spark

pull latest from apache spark

Pulling functionality from apache spark

pull request from apache/master

pull latest from apache spark

Pull apache spark

pull latest apache spark

Apache spark pull latest

SparkQA · 2018-01-21T07:39:01Z

Test build #86423 has finished for PR 20338 at commit 8873441.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-01-21T08:43:00Z

This doesn't actually run the Python lint in Jenkins build though because there's no change on the files with .py. Mind double checking and fixing:

diff --git a/dev/run-tests.py b/dev/run-tests.py
index 7e6f7ff0603..6fac324972e 100755
--- a/dev/run-tests.py
+++ b/dev/run-tests.py
@@ -576,7 +576,10 @@ def main():
                                 for f in changed_files):
         # run_java_style_checks()
         pass
-    if not changed_files or any(f.endswith(".py") for f in changed_files):
+    if not changed_files or any(f.endswith("lint-python")
+                                or f.endswith("tox.ini")
+                                or f.endswith(".py")
+                                for f in changed_files):
         run_python_style_checks()
     if not changed_files or any(f.endswith(".R") for f in changed_files):
         run_sparkr_style_checks()

too?

HyukjinKwon · 2018-01-21T08:46:12Z

dev/tox.ini

-[pep8]
-ignore=E402,E731,E241,W503,E226
+[pycodestyle]
+ignore=E402,E731,E241,W503,E226,E722,E741,E305


So, do those E722,E741,E305 include rules to validate the blank line before starting doctests, which is described in the JIRA?

Seems:
E722: "do not use bare except'"
E741: "ambiguous variable name 'l'",
E305: "expected 2 blank lines after class or function definition"

These(E722, E741, E305) are new code style standards added that Spark seems to be violating.For now, added them in ignored.I have raised the doctest blank line as an issue under pep8, as it does not belong within Spark.

Ah, I asked this to double check if this PR fixes the issue linked JIRA. I think we could make another JIRA if this PR doesn't target to fix the linked JIRA.

For the best, it'd be nicer if you'd leave a issue link you raised under pep8 in SPARK-11222, and open another JIRA about what this PR changes and then link it by fixing the PR title, if I understood you correctly.

Updated SPARK-11222. Added https://issues.apache.org/jira/browse/SPARK-23174.

should we fix the code that violates

E722: "do not use bare except'"
E741: "ambiguous variable name 'l'",
E305: "expected 2 blank lines after class or function definition"
?

Or that's another PR/JIRA?

Yea .. there look actually many instances about it. I manually ran it against this PR. Will maybe take this one and open a PR separately after double checking each rule. Didn't take a look carefully yet.

HyukjinKwon · 2018-01-21T08:48:39Z

dev/lint-python

+PEP8_VERSION="2.3.1"
 PEP8_SCRIPT_PATH="$SPARK_ROOT_DIR/dev/pep8-$PEP8_VERSION.py"
-PEP8_SCRIPT_REMOTE_PATH="https://raw.githubusercontent.com/jcrocholl/pep8/$PEP8_VERSION/pep8.py"
+PEP8_SCRIPT_REMOTE_PATH="https://raw.githubusercontent.com/PyCQA/pycodestyle/$PEP8_VERSION/pycodestyle.py"


Shall we leave a note that pep8 is formally renamed to pycodestyle to reduce confusion, and in the PR description too?

rekhajoshm · 2018-01-22T01:44:43Z

Thanks @HyukjinKwon for your review.updated, please verify.

HyukjinKwon

LGTM otherwise.

HyukjinKwon · 2018-01-22T01:55:51Z

dev/run-tests.py

        sys.exit(int(os.environ.get("CURRENT_BLOCK", 255)))
    else:
        run_cmd([jekyll_bin, "build"])
-


I know I am insane but let's revert back unrelated changes.

SparkQA · 2018-01-22T05:29:45Z

Test build #86451 has finished for PR 20338 at commit 8718891.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

ueshin

LGTM except one question.

ueshin · 2018-01-22T05:40:14Z

dev/lint-python

-#+  - Download pep8 from PyPI. It's more "official".
-PEP8_VERSION="1.7.0"
+# Updated to latest official version for pep8.pep8 is formally renamed to pycodestyle.
+PEP8_VERSION="2.3.1"


Should we also use pycodestyle instead of pep8 for variable names or script names?

SparkQA · 2018-01-22T06:33:02Z

Test build #86454 has finished for PR 20338 at commit c386a9a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-01-23T13:34:36Z

dev/lint-python

-#+ TODOs:
-#+  - Download pep8 from PyPI. It's more "official".
-PEP8_VERSION="1.7.0"
+# Updated to latest official version for pep8.pep8 is formally renamed to pycodestyle.


nit .. : pep8.pep8 -> pep8. pep8

srowen · 2018-01-23T14:18:43Z

(If this also resolves SPARK-11222 you can also include that in the title.)

HyukjinKwon · 2018-01-24T02:04:36Z

Ah, seems this one doesn't resolve SPARK-11222 too. I think that JIRA describes to add pydocstyle check in our project too given my reading of @rekhajoshm's issue link. Will double check and take an action for SPARK-11222 soon.

This JIRA, SPARK-23174, seems describing simply upgrading the pep8 (formally renamed to pycodestyle).

HyukjinKwon · 2018-01-24T02:05:04Z

@rekhajoshm, mind updating variable names and one nit above? Will merge this one as soon as they are fixed.

rekhajoshm · 2018-01-24T03:38:15Z

@HyukjinKwon @ueshin was bit hesitant to change variable/references as pep8 is very clear within python circles as python style checker.however after pondering, have done the needful. please check.thanks

@srowen for SPARK-11222, I had raised a issue under pycodestyle. Created PR #20378

SparkQA · 2018-01-24T06:38:45Z

Test build #86559 has finished for PR 20338 at commit d10fbb4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-24T06:40:03Z

Test build #86560 has finished for PR 20338 at commit 34a8590.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-01-24T06:44:07Z

retest this please

SparkQA · 2018-01-24T08:05:01Z

Test build #86569 has finished for PR 20338 at commit 34a8590.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-01-24T08:06:56Z

retest this please

SparkQA · 2018-01-24T12:02:13Z

Test build #86578 has finished for PR 20338 at commit 34a8590.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-01-24T12:14:15Z

Merged to master.

…ore file. ## What changes were proposed in this pull request? This is a follow-up pr of apache#20338 which changed the downloaded file name of the python code style checker but it's not contained in .gitignore file so the file remains as an untracked file for git after running the checker. This pr adds the file name to .gitignore file. ## How was this patch tested? Tested manually. Author: Takuya UESHIN <[email protected]> Closes apache#20432 from ueshin/issues/SPARK-23174/fup1.

rekhajoshm added 11 commits May 5, 2015 16:10

Merge pull request #1 from apache/master

e3677c9

Pulling functionality from apache spark

Merge pull request #2 from apache/master

106fd8e

pull latest from apache spark

Merge pull request #3 from apache/master

0be142d

Pulling functionality from apache spark

Merge pull request #4 from apache/master

6c6ee12

Pulling functionality from apache spark

Merge pull request #5 from apache/master

b123c60

pull request from apache/master

Merge pull request #6 from apache/master

c73c32a

pull latest from apache spark

Merge pull request #8 from apache/master

7dbf732

pull latest from apache spark

Merge pull request #9 from apache/master

5e9d718

Pull apache spark

Merge pull request #10 from apache/master

63d99b3

pull latest apache spark

Merge pull request #11 from apache/master

a7fc787

Apache spark pull latest

[SPARK-11222][BUILD][PYTHON] python code style checker update

8873441

HyukjinKwon reviewed Jan 21, 2018

View reviewed changes

updated for review comment

8718891

HyukjinKwon reviewed Jan 22, 2018

View reviewed changes

[SPARK-23174][BUILD][PYTHON] python code style checker update

c386a9a

rekhajoshm changed the title ~~[SPARK-11222][BUILD][PYTHON] python code style checker update~~ [SPARK-23174][BUILD][PYTHON] python code style checker update Jan 22, 2018

ueshin reviewed Jan 22, 2018

View reviewed changes

HyukjinKwon reviewed Jan 23, 2018

View reviewed changes

rekhajoshm added 2 commits January 23, 2018 19:17

updated nit

d10fbb4

updated for review comment

34a8590

asfgit closed this in 7af1a32 Jan 24, 2018

ueshin mentioned this pull request Jan 30, 2018

[SPARK-23174][BUILD][PYTHON][FOLLOWUP] Add pycodestyle*.py to .gitignore file. #20432

Closed

viirya mentioned this pull request Jan 30, 2018

[SPARK-11222][Build][Python] Python document style checker added #20378

Closed

[SPARK-23174][BUILD][PYTHON] python code style checker update #20338

[SPARK-23174][BUILD][PYTHON] python code style checker update #20338

Uh oh!

Conversation

rekhajoshm commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jan 21, 2018

Uh oh!

HyukjinKwon commented Jan 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rekhajoshm Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rekhajoshm commented Jan 22, 2018

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 22, 2018

Uh oh!

ueshin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen commented Jan 23, 2018

Uh oh!

HyukjinKwon commented Jan 24, 2018

Uh oh!

HyukjinKwon commented Jan 24, 2018

Uh oh!

rekhajoshm commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jan 24, 2018

Uh oh!

SparkQA commented Jan 24, 2018

Uh oh!

HyukjinKwon commented Jan 24, 2018

Uh oh!

SparkQA commented Jan 24, 2018

Uh oh!

HyukjinKwon commented Jan 24, 2018

Uh oh!

SparkQA commented Jan 24, 2018

Uh oh!

HyukjinKwon commented Jan 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rekhajoshm commented Jan 21, 2018 •

edited

Loading

rekhajoshm Jan 22, 2018 •

edited

Loading

HyukjinKwon Jan 22, 2018 •

edited

Loading

rekhajoshm commented Jan 24, 2018 •

edited

Loading