Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rekhajoshm wants to merge 21 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
e3677c9
Merge pull request #1 from apache/master
rekhajoshm May 5, 2015
106fd8e
Merge pull request #2 from apache/master
rekhajoshm May 8, 2015
0be142d
Merge pull request #3 from apache/master
rekhajoshm Jun 22, 2015
6c6ee12
Merge pull request #4 from apache/master
rekhajoshm Sep 17, 2015
b123c60
Merge pull request #5 from apache/master
rekhajoshm Nov 25, 2015
c73c32a
Merge pull request #6 from apache/master
rekhajoshm Mar 18, 2016
7dbf732
Merge pull request #8 from apache/master
rekhajoshm Apr 5, 2016
5e9d718
Merge pull request #9 from apache/master
rekhajoshm May 1, 2017
63d99b3
Merge pull request #10 from apache/master
rekhajoshm Sep 30, 2017
a7fc787
Merge pull request #11 from apache/master
rekhajoshm Jan 21, 2018
3a2d453
Merge pull request #12 from apache/master
rekhajoshm Feb 9, 2018
dca3a9e
Merge pull request #13 from apache/master
rekhajoshm May 31, 2018
fe58c37
Merge remote-tracking branch 'upstream/master'
rekhajoshm Jun 21, 2018
ae51f60
Merge pull request #14 from apache/master
rekhajoshm Jun 21, 2018
1cf4ed2
Merge branch 'master' of https://github.com/rekhajoshm/spark
rekhajoshm Jun 30, 2018
1c48d4f
Merge pull request #15 from apache/master
rekhajoshm Jun 30, 2018
aa735f5
Merge branch 'master' of https://github.com/rekhajoshm/spark
rekhajoshm Jun 30, 2018
9356507
Merge pull request #16 from apache/master
rekhajoshm Sep 15, 2018
64b2eb2
Merge branch 'master' of https://github.com/rekhajoshm/spark
rekhajoshm Sep 15, 2018
af83d90
[SPARK-23367][Build] Include python document style checking
rekhajoshm Sep 15, 2018
cba18e8
updated for pydocstyle latest version, and for review comments
rekhajoshm Oct 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions dev/lint-python
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,14 @@ SCRIPT_DIR="$( cd "$( dirname "$0" )" && pwd )"
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' )"
PYCODESTYLE_REPORT_PATH="$SPARK_ROOT_DIR/dev/pycodestyle-report.txt"
PYDOCSTYLE_REPORT_PATH="$SPARK_ROOT_DIR/dev/pydocstyle-report.txt"
PYLINT_REPORT_PATH="$SPARK_ROOT_DIR/dev/pylint-report.txt"
PYLINT_INSTALL_INFO="$SPARK_ROOT_DIR/dev/pylint-info.txt"
PYDOCSTYLEBUILD="pydocstyle"
EXPECTED_PYDOCSTYLEVERSION="2.1.1"
PYDOCSTYLEVERSION=$(python -c 'import pkg_resources; print(pkg_resources.get_distribution("pydocstyle").version)' 2> /dev/null)
SPHINXBUILD=${SPHINXBUILD:=sphinx-build}
SPHINX_REPORT_PATH="$SPARK_ROOT_DIR/dev/sphinx-report.txt"

Expand Down Expand Up @@ -99,6 +104,29 @@ else
echo "flake8 checks passed."
fi

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

@srowen srowen Oct 22, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@srowen srowen Oct 27, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if hash "$PYDOCSTYLEBUILD" 2> /dev/null; then
if [[ "$PYDOCSTYLEVERSION" == "$EXPECTED_PYDOCSTYLEVERSION" ]]; then
pydocstyle --config=dev/tox.ini $DOC_PATHS_TO_CHECK >> "$PYDOCSTYLE_REPORT_PATH"
pydocstyle_status="${PIPESTATUS[0]}"

if [ "$compile_status" -eq 0 -a "$pydocstyle_status" -eq 0 ]; then
echo "pydocstyle checks passed."
rm "$PYDOCSTYLE_REPORT_PATH"
else
echo "pydocstyle checks failed."
cat "$PYDOCSTYLE_REPORT_PATH"
rm "$PYDOCSTYLE_REPORT_PATH"
exit 1
fi

else
echo "The pydocstyle version needs to be latest 2.1.1. Skipping pydoc checks for now"
fi
else
echo >&2 "The pydocstyle command was not found. Skipping pydoc checks for now"
fi

# Check that the documentation builds acceptably, skip check if sphinx is not installed.
if hash "$SPHINXBUILD" 2> /dev/null; then
cd python/docs
Expand Down
4 changes: 3 additions & 1 deletion dev/tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
# limitations under the License.

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

@srowen srowen Sep 15, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rekhajoshm rekhajoshm Sep 26, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@holdenk holdenk Sep 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@srowen srowen Sep 15, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rekhajoshm rekhajoshm Sep 26, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@holdenk holdenk Sep 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ueshin ueshin Sep 27, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@srowen srowen Oct 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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