Skip to content

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

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 15 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 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
8873441
[SPARK-11222][BUILD][PYTHON] python code style checker update
rekhajoshm Jan 21, 2018
8718891
updated for review comment
rekhajoshm Jan 22, 2018
c386a9a
[SPARK-23174][BUILD][PYTHON] python code style checker update
rekhajoshm Jan 22, 2018
d10fbb4
updated nit
rekhajoshm Jan 24, 2018
34a8590
updated for review comment
rekhajoshm Jan 24, 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
37 changes: 18 additions & 19 deletions dev/lint-python
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ 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" )"
PEP8_REPORT_PATH="$SPARK_ROOT_DIR/dev/pep8-report.txt"
PYCODESTYLE_REPORT_PATH="$SPARK_ROOT_DIR/dev/pycodestyle-report.txt"
PYLINT_REPORT_PATH="$SPARK_ROOT_DIR/dev/pylint-report.txt"
PYLINT_INSTALL_INFO="$SPARK_ROOT_DIR/dev/pylint-info.txt"
SPHINXBUILD=${SPHINXBUILD:=sphinx-build}
Expand All @@ -30,23 +30,22 @@ SPHINX_REPORT_PATH="$SPARK_ROOT_DIR/dev/sphinx-report.txt"
cd "$SPARK_ROOT_DIR"

# compileall: https://docs.python.org/2/library/compileall.html
python -B -m compileall -q -l $PATHS_TO_CHECK > "$PEP8_REPORT_PATH"
python -B -m compileall -q -l $PATHS_TO_CHECK > "$PYCODESTYLE_REPORT_PATH"
compile_status="${PIPESTATUS[0]}"

# Get pep8 at runtime so that we don't rely on it being installed on the build server.
# Get pycodestyle at runtime so that we don't rely on it being installed on the build server.
#+ See: https://github.com/apache/spark/pull/1744#issuecomment-50982162
#+ TODOs:
#+ - Download pep8 from PyPI. It's more "official".
PEP8_VERSION="1.7.0"
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"
# Updated to latest official version for pep8. pep8 is formally renamed to pycodestyle.
PYCODESTYLE_VERSION="2.3.1"
PYCODESTYLE_SCRIPT_PATH="$SPARK_ROOT_DIR/dev/pycodestyle-$PYCODESTYLE_VERSION.py"
PYCODESTYLE_SCRIPT_REMOTE_PATH="https://raw.githubusercontent.com/PyCQA/pycodestyle/$PYCODESTYLE_VERSION/pycodestyle.py"

if [ ! -e "$PEP8_SCRIPT_PATH" ]; then
curl --silent -o "$PEP8_SCRIPT_PATH" "$PEP8_SCRIPT_REMOTE_PATH"
if [ ! -e "$PYCODESTYLE_SCRIPT_PATH" ]; then
curl --silent -o "$PYCODESTYLE_SCRIPT_PATH" "$PYCODESTYLE_SCRIPT_REMOTE_PATH"
curl_status="$?"

if [ "$curl_status" -ne 0 ]; then
echo "Failed to download pep8.py from \"$PEP8_SCRIPT_REMOTE_PATH\"."
echo "Failed to download pycodestyle.py from \"$PYCODESTYLE_SCRIPT_REMOTE_PATH\"."
exit "$curl_status"
fi
fi
Expand All @@ -64,23 +63,23 @@ export "PATH=$PYTHONPATH:$PATH"
#+ first, but we do so so that the check status can
#+ be output before the report, like with the
#+ scalastyle and RAT checks.
python "$PEP8_SCRIPT_PATH" --config=dev/tox.ini $PATHS_TO_CHECK >> "$PEP8_REPORT_PATH"
pep8_status="${PIPESTATUS[0]}"
python "$PYCODESTYLE_SCRIPT_PATH" --config=dev/tox.ini $PATHS_TO_CHECK >> "$PYCODESTYLE_REPORT_PATH"
pycodestyle_status="${PIPESTATUS[0]}"

if [ "$compile_status" -eq 0 -a "$pep8_status" -eq 0 ]; then
if [ "$compile_status" -eq 0 -a "$pycodestyle_status" -eq 0 ]; then
lint_status=0
else
lint_status=1
fi

if [ "$lint_status" -ne 0 ]; then
echo "PEP8 checks failed."
cat "$PEP8_REPORT_PATH"
rm "$PEP8_REPORT_PATH"
echo "PYCODESTYLE checks failed."
cat "$PYCODESTYLE_REPORT_PATH"
rm "$PYCODESTYLE_REPORT_PATH"
exit "$lint_status"
else
echo "PEP8 checks passed."
rm "$PEP8_REPORT_PATH"
echo "pycodestyle checks passed."
rm "$PYCODESTYLE_REPORT_PATH"
fi

# Check that the documentation builds acceptably, skip check if sphinx is not installed.
Expand Down
5 changes: 4 additions & 1 deletion dev/run-tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
4 changes: 2 additions & 2 deletions dev/tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

[pep8]
ignore=E402,E731,E241,W503,E226
[pycodestyle]
ignore=E402,E731,E241,W503,E226,E722,E741,E305
Copy link
Member

@HyukjinKwon HyukjinKwon Jan 21, 2018

Choose a reason for hiding this comment

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

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"

Copy link
Contributor Author

@rekhajoshm rekhajoshm Jan 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@HyukjinKwon HyukjinKwon Jan 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@rekhajoshm rekhajoshm Jan 22, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@felixcheung felixcheung Jan 24, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Member

@HyukjinKwon HyukjinKwon Jan 24, 2018

Choose a reason for hiding this comment

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

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.

max-line-length=100
exclude=cloudpickle.py,heapq3.py,shared.py,python/docs/conf.py,work/*/*.py,python/.eggs/*