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

rekhajoshm · 2018-09-15T02:54:34Z

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 3.0.0 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

apache spark latest pull

latest apache spark

apache spark latest

Apache Spark master latest

SparkQA · 2018-09-15T07:05:02Z

Test build #96086 has finished for PR 22425 at commit af83d90.

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

srowen

I like the general idea, but this also turns off most checks? are there any that can be meaningfully re-enabled with a few fixes?

srowen · 2018-09-15T20:53:34Z

dev/tox.ini


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


Do we need to exclude 605? I thought we fixed all those...

The intent of this PR was on pydocstyle changes, and this was pycodestyle break, so was unsure if that fix can be included in this PR. I had seen W605 code style broken in that build, and to make it pass added this. I agree completely the fix for W605 can be done with no-PR trivial fix., will check again.

I'm just confused why this would need to be changed in this PR -- hopefully just a hold over from the previous PR?

srowen · 2018-09-15T20:53:54Z

dev/tox.ini

 max-line-length=100
 exclude=cloudpickle.py,heapq3.py,shared.py,python/docs/conf.py,work/*/*.py,python/.eggs/*,dist/*
+[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


This is tiny, but is this blank line necessary? it seems to separate the heading from its content?

Between [pydocstyle] to ignore is necessary and standard style. Post ignore the blank line was added as per requested by last reviewer on previous PR - #20556 (comment)

I don't think that's what @ueshin was asking for, I think it was a blank line after the ignore=..., but if @ueshin is around we can see what @ueshin says. It's also relatively minor provided everything functions.

I just asked to add a line break at the end of file, and the current style looks good to me.

EDIT: disregard, there isn't a blank line, it's just github display confusing me. Oops, sorry about that.

HyukjinKwon

I would also leave short explanations in PR description to justify why they should be skipped. Looks so many things are being skipped. FYI, this project does not yet claim PEP 257 as far as I know.

rekhajoshm · 2018-09-26T23:54:24Z

As per earlier discussions with prior reviewers, the intent of this PR was to make changes to enable pydocstyle but not to include all pydocstyle changes across all docs as that would need to be discussed later and be prioritized. I will relook at this again with your comments and check pydoc for style corrections.Thanks.

Sorry @srowen @HyukjinKwon , I was waiting from Feb2018 to Jul2018 on the previous PR #20556 , and hence there is a bit of disconnect from this issue. Plus bit caught up at work and hence delay in providing context in which this PR was created.

srowen

Ok I think it's fine to focus on enabling this tool here and then making changes later as we unexclude warnings.

I think we had separately fixed the w605 errors; might be worth checking if that's still needed.

holdenk

Thanks for reviving this PR I'm excited to get better style checkers integrated into Spark. My main concern is with the change to pycodestyle's list of excluded problems.

holdenk · 2018-09-27T14:37:59Z

dev/tox.ini

 max-line-length=100
 exclude=cloudpickle.py,heapq3.py,shared.py,python/docs/conf.py,work/*/*.py,python/.eggs/*,dist/*
+[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


I don't think that's what @ueshin was asking for, I think it was a blank line after the ignore=..., but if @ueshin is around we can see what @ueshin says. It's also relatively minor provided everything functions.

holdenk · 2018-09-27T14:38:31Z

dev/tox.ini


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


I'm just confused why this would need to be changed in this PR -- hopefully just a hold over from the previous PR?

holdenk · 2018-10-19T16:30:55Z

Gentle ping, whats up?

rekhajoshm · 2018-10-19T20:32:50Z

Thank you @srowen @holdenk @HyukjinKwon for your review.

As per discussion, in this PR, we are enabling docstyle change but not updating pydocs. Hence tox.ini has doc error codes included.It can be taken off few at a time.

pydocstyle is newer correct way of handling doc style issues over pep257. It covers both pep257 and numpy convention style. http://www.pydocstyle.org/en/3.0.0/error_codes.html

updated pydocstyle to latest 3.0.0.
removed W605 from tox.ini. (the breaking codestyle error(W605) had appeared in the build when this branch was created, hence was quickly added to pass this, not lingering from old PR.)

SparkQA · 2018-10-20T00:21:59Z

Test build #97621 has finished for PR 22425 at commit cba18e8.

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

SparkQA · 2018-10-22T13:13:01Z

Test build #97820 has started for PR 22425 at commit cba18e8.

srowen · 2018-10-22T14:19:40Z

dev/lint-python

    echo "flake8 checks passed."
 fi

+# Check python document style, skip check if pydocstyle is not installed.


Nit: the indentation below looks different (4-space) from the rest of the script (2-space)

Actually, seems like indentation is inconsistent in this file, so let's leave it for now

srowen · 2018-10-22T14:20:31Z

dev/tox.ini

 max-line-length=100
 exclude=cloudpickle.py,heapq3.py,shared.py,python/docs/conf.py,work/*/*.py,python/.eggs/*,dist/*
+[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


EDIT: disregard, there isn't a blank line, it's just github display confusing me. Oops, sorry about that.

SparkQA · 2018-10-22T14:25:59Z

Test build #97844 has started for PR 22425 at commit cba18e8.

SparkQA · 2018-10-22T14:30:48Z

Test build #97836 has finished for PR 22425 at commit cba18e8.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

holdenk · 2018-10-26T16:28:05Z

I think the scaladoc error is unrelated, jenkins retest this please.

SparkQA · 2018-10-26T20:33:09Z

Test build #98096 has finished for PR 22425 at commit cba18e8.

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

srowen · 2018-10-27T13:21:39Z

Merged to master. Anyone who feels like then addressing the style warnings, some that may be easy to address, go ahead.

## 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 3.0.0 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 Closes apache#22425 from rekhajoshm/SPARK-23367-2. Authored-by: Rekha Joshi <[email protected]> Signed-off-by: Sean Owen <[email protected]>

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

Merge pull request #13 from apache/master

dca3a9e

apache spark latest pull

Merge remote-tracking branch 'upstream/master'

fe58c37

Merge pull request #14 from apache/master

ae51f60

latest apache spark

Merge branch 'master' of https://github.com/rekhajoshm/spark

1cf4ed2

Merge pull request #15 from apache/master

1c48d4f

apache spark latest

Merge branch 'master' of https://github.com/rekhajoshm/spark

aa735f5

Merge pull request #16 from apache/master

9356507

Apache Spark master latest

Merge branch 'master' of https://github.com/rekhajoshm/spark

64b2eb2

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

af83d90

rekhajoshm mentioned this pull request Sep 15, 2018

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

Closed

srowen reviewed Sep 15, 2018

View reviewed changes

HyukjinKwon reviewed Sep 17, 2018

View reviewed changes

srowen reviewed Sep 27, 2018

View reviewed changes

holdenk reviewed Sep 27, 2018

View reviewed changes

updated for pydocstyle latest version, and for review comments

cba18e8

srowen reviewed Oct 22, 2018

View reviewed changes

asfgit closed this in d5573c5 Oct 27, 2018

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

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

Uh oh!

Conversation

rekhajoshm commented Sep 15, 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 Sep 15, 2018

Uh oh!

srowen 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!

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!

srowen Oct 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 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

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 Oct 19, 2018

Uh oh!

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

Uh oh!

Uh oh!

SparkQA commented Oct 20, 2018

Uh oh!

SparkQA commented Oct 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen Oct 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!

SparkQA commented Oct 22, 2018

Uh oh!

SparkQA commented Oct 22, 2018

Uh oh!

holdenk commented Oct 26, 2018

Uh oh!

SparkQA commented Oct 26, 2018

Uh oh!

srowen commented Oct 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

rekhajoshm commented Sep 15, 2018 •

edited

Loading

srowen Oct 22, 2018 •

edited

Loading

rekhajoshm commented Sep 26, 2018 •

edited

Loading

rekhajoshm commented Oct 19, 2018 •

edited

Loading

srowen Oct 22, 2018 •

edited

Loading