[SPARK-23367][Build] Include python document style checking #20556

rekhajoshm · 2018-02-09T05:58:31Z

What changes were proposed in this pull request?

Includes python document style checking.

Use sphinx like check, run only if pydocstyle installed on machine/jenkins
use pydocstyle rather than single file version pep257.py, which is much older and had some known issues
verify pydocstyle latest 2.1.1 is in use, to ensure latest doc checks are getting executed
ignore (inclusion/exclusion error codes) features and support via tox.ini
Be non-breaking change and allow updating docstyle to standards at easy pace

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

Apache spark latest pull

SparkQA · 2018-02-09T06:03:47Z

Test build #87248 has finished for PR 20556 at commit 85ca69d.

This patch fails RAT tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-02-09T08:05:01Z

Test build #87249 has finished for PR 20556 at commit ee14cf7.

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

rekhajoshm · 2018-02-14T04:21:03Z

build error unrelated to this PR. retest this please.

ueshin · 2018-02-14T05:16:05Z

dev/lint-python

 SPARK_ROOT_DIR="$(dirname "$SCRIPT_DIR")"
 # Exclude auto-generated configuration file.
 PATHS_TO_CHECK="$( cd "$SPARK_ROOT_DIR" && find . -name "*.py" )"
+DOC_PATHS_TO_CHECK="$( cd "$SPARK_ROOT_DIR" && find . -name "*.py" | grep -vF 'functions.py')"


nit: add an extra space between 'functions.py' and ).

ueshin · 2018-02-14T05:16:49Z

dev/lint-python

 PYLINT_REPORT_PATH="$SPARK_ROOT_DIR/dev/pylint-report.txt"
 PYLINT_INSTALL_INFO="$SPARK_ROOT_DIR/dev/pylint-info.txt"
+PYDOCSTYLEBUILD="pydocstyle"
+PYDOCSTYLEVERSION=$(python -c 'import pkg_resources; print pkg_resources.get_distribution("pydocstyle").version')


..; print(pkg_resources.get_distribution("pydocstyle").version)' 2> /dev/null)

ueshin · 2018-02-14T05:17:31Z

dev/tox.ini

 max-line-length=100
 exclude=cloudpickle.py,heapq3.py,shared.py,python/docs/conf.py,work/*/*.py,python/.eggs/*
+[pydocstyle]
+ignore=D100,D101,D102,D103,D104,D105,D106,D107,D200,D201,D202,D203,D204,D205,D206,D207,D208,D209,D210,D211,D212,D213,D214,D215,D300,D301,D302,D400,D401,D402,D403,D404,D405,D406,D407,D408,D409,D410,D411,D412,D413,D414


We need a line break at the end of the file.

SparkQA · 2018-02-14T07:24:11Z

Test build #87439 has finished for PR 20556 at commit ee14cf7.

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

BryanCutler · 2018-02-14T18:13:19Z

dev/lint-python


+# Check python document style, skip check if pydocstyle is not installed.
+if hash "$PYDOCSTYLEBUILD" 2> /dev/null; then
+    if [ $PYDOCSTYLEVERSION == "2.1.1" ]; then


What happens when a new version of pydocstyle is released? Would the same rules below still work so that this check could be >= 2.1.1?

BryanCutler · 2018-02-14T18:13:24Z

dev/lint-python

+            lint_status=1
+        fi
+
+        if [ "$lint_status" -ne 0 ]; then


could you just move this block above and get rid of lint_status?

not sure i understand

So if I understand @BryanCutler's request correctly, instead of setting linte_status in the if check above and then lint_status to determine if we should run the pydocstyle checker, just directly check the compile status and then run the pydocstyle checker on that.

That being said I think we could also drop the compile_status check since we would have already exited the script if it was non-zero at this point (I think).

holdenk · 2018-02-26T22:44:08Z

Thanks for working on this! I like the idea of automated document style checking for Python :)

holdenk · 2018-02-26T22:44:42Z

Jenkins retest this please.

SparkQA · 2018-02-27T02:32:30Z

Test build #87683 has finished for PR 20556 at commit ee14cf7.

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

SparkQA · 2018-02-27T05:23:42Z

Test build #87688 has finished for PR 20556 at commit 11ad2c1.

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

holdenk

Thanks for working on this. Is this a thing you still have bandwidth to work on?

holdenk · 2018-07-27T18:30:55Z

dev/lint-python

+            lint_status=1
+        fi
+
+        if [ "$lint_status" -ne 0 ]; then


So if I understand @BryanCutler's request correctly, instead of setting linte_status in the if check above and then lint_status to determine if we should run the pydocstyle checker, just directly check the compile status and then run the pydocstyle checker on that.

That being said I think we could also drop the compile_status check since we would have already exited the script if it was non-zero at this point (I think).

holdenk · 2018-07-27T18:31:15Z

dev/lint-python

+            rm "$PYDOCSTYLE_REPORT_PATH"
+        fi
+    else
+        echo "The pydocstyle version needs to be latest 2.1.1.Skipping pydoc checks for now"


nit: add a space between "2.1.1.Skipping" the final dot and skipping.

holdenk · 2018-07-27T18:32:01Z

If your still working on this would be good to merge in the latest master branch.

holdenk · 2018-09-14T16:42:32Z

Are you still interested in this PR?

rekhajoshm · 2018-09-14T18:15:24Z

Yes @holdenk , seems like I missed the last notification on it. Please let me look into it this weekend. Thanks.

rekhajoshm · 2018-09-15T02:56:46Z

PR #22425 Closing this.Thanks

SparkQA · 2018-09-15T06:07:12Z

Test build #96085 has finished for PR 20556 at commit 5f6b409.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

rekhajoshm added 12 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

Merge pull request #12 from apache/master

3a2d453

Apache spark latest pull

[SPARK-23367][Build] Include python document style checking

85ca69d

[SPARK-23367][Build] Include python document style checking

ee14cf7

rekhajoshm mentioned this pull request Feb 9, 2018

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

Closed

ueshin reviewed Feb 14, 2018

View reviewed changes

BryanCutler reviewed Feb 14, 2018

View reviewed changes

updated for review comments

11ad2c1

holdenk reviewed Jul 27, 2018

View reviewed changes

updated for review comments

5f6b409

rekhajoshm closed this Sep 15, 2018

rekhajoshm mentioned this pull request Sep 26, 2018

[SPARK-23367][Build] Include python document style checking #22425

Closed

[SPARK-23367][Build] Include python document style checking #20556

[SPARK-23367][Build] Include python document style checking #20556

Uh oh!

Conversation

rekhajoshm commented Feb 9, 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 Feb 9, 2018

Uh oh!

SparkQA commented Feb 9, 2018

Uh oh!

rekhajoshm commented Feb 14, 2018

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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 14, 2018

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!

holdenk commented Feb 26, 2018

Uh oh!

holdenk commented Feb 26, 2018

Uh oh!

SparkQA commented Feb 27, 2018

Uh oh!

SparkQA commented Feb 27, 2018

Uh oh!

holdenk left a comment

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!

holdenk commented Jul 27, 2018

Uh oh!

holdenk commented Sep 14, 2018

Uh oh!

rekhajoshm commented Sep 14, 2018

Uh oh!

rekhajoshm commented Sep 15, 2018

Uh oh!

SparkQA commented Sep 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rekhajoshm commented Feb 9, 2018 •

edited

Loading