Skip to content
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.

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

[SPARK-25270] lint-python: Add flake8 to find syntax errors and undefined names #22266

Closed
wants to merge 4 commits into from

Conversation

cclauss
Copy link

@cclauss cclauss commented Aug 29, 2018

What changes were proposed in this pull request?

Add flake8 tests to find Python syntax errors and undefined names.

E901,E999,F821,F822,F823 are the "showstopper" flake8 issues that can halt the runtime with a SyntaxError, NameError, etc. Most other flake8 issues are merely "style violations" -- useful for readability but they do not effect runtime safety.

  • F821: undefined name name
  • F822: undefined name name in __all__
  • F823: local variable name referenced before assignment
  • E901: SyntaxError or IndentationError
  • E999: SyntaxError -- failed to compile a file into an Abstract Syntax Tree

How was this patch tested?

$ flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics
$ flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics

Please review http://spark.apache.org/contributing.html before opening a pull request.

…ned names

Add [flake8](http://flake8.pycqa.org) tests to find Python syntax errors and undefined names.

__E901,E999,F821,F822,F823__ are the "_showstopper_" flake8 issues that can halt the runtime with a SyntaxError, NameError, etc. Most other flake8 issues are merely "style violations" -- useful for readability but they do not effect runtime safety.
* F821: undefined name `name`
* F822: undefined name `name` in `__all__`
* F823: local variable name referenced before assignment
* E901: SyntaxError or IndentationError
* E999: SyntaxError -- failed to compile a file into an Abstract Syntax Tree
@cclauss cclauss changed the title lint-python: Add flake8 tests to find Python syntax errors and undefi… lint-python: Add flake8 to find syntax errors and undefined names Aug 29, 2018
@cclauss
Copy link
Author

cclauss commented Aug 29, 2018

These tests should fail until #22265 or similar is merged.

@HyukjinKwon
Copy link
Member

Can you file a JIRA please?

@cclauss cclauss changed the title lint-python: Add flake8 to find syntax errors and undefined names [SPARK-25270] lint-python: Add flake8 to find syntax errors and undefined names Aug 29, 2018
@viirya
Copy link
Member

viirya commented Aug 29, 2018

Can you also format the PR description to follow the template?

@cclauss
Copy link
Author

cclauss commented Aug 29, 2018

@HyukjinKwon Done.
@viirya Done.

@HyukjinKwon
Copy link
Member

ok to test

@SparkQA
Copy link

SparkQA commented Aug 30, 2018

Test build #95447 has finished for PR 22266 at commit 1291198.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

dev/lint-python Outdated Show resolved Hide resolved
dev/lint-python Outdated Show resolved Hide resolved
@SparkQA
Copy link

SparkQA commented Aug 31, 2018

Test build #95501 has finished for PR 22266 at commit c8c0543.

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

dev/lint-python Outdated Show resolved Hide resolved
Copy link
Contributor

@holdenk holdenk left a comment

Choose a reason for hiding this comment

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

Thanks for doing the cleanup as well as the follow up PR to make sure we don't regress. Some minor questions too :)

dev/lint-python Outdated Show resolved Hide resolved
@SparkQA
Copy link

SparkQA commented Aug 31, 2018

Test build #95555 has finished for PR 22266 at commit b87c49e.

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

@SparkQA
Copy link

SparkQA commented Aug 31, 2018

Test build #95556 has finished for PR 22266 at commit 786d0c9.

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

@holdenk
Copy link
Contributor

holdenk commented Sep 7, 2018

This LGTM but my only problem is we introduced another undefined name in the java_gateway in the meantime. I'll fix that today and merge this PR.

@holdenk
Copy link
Contributor

holdenk commented Sep 7, 2018

Oh wait that was fixed in master I just had this branch checked out. Verified flake8 cmd against f96a8bf so we should be fine to merge to master without breaking anything. LGTM

@asfgit asfgit closed this in 22a46ca Sep 7, 2018
@holdenk
Copy link
Contributor

holdenk commented Sep 7, 2018

Merged to master, it's not in branch-2.4 (although if folks think in belongs there I'm open to that). I haven't closed the JIRA yet because we don't have the version tag in JIRA for the next release after 2.4.

@cclauss cclauss deleted the patch-3 branch September 7, 2018 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants