You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/01/07 06:13:20 UTC
[GitHub] [spark] HyukjinKwon opened a new pull request #35126: [SPARK-37836][PYTHON][INFRA] Enable F841, E722, E305 and E226 for PEP 8 compliance
HyukjinKwon opened a new pull request #35126:
URL: https://github.com/apache/spark/pull/35126
### What changes were proposed in this pull request?
This PR proposes to enable flake8 rules as below:
- [F841](https://www.flake8rules.com/rules/F841.html): Local variable name is assigned to but never used
- [E722](https://www.flake8rules.com/rules/E722.html): Do not use bare except, specify exception instead
- [E305](https://www.flake8rules.com/rules/E305.html): Expected 2 blank lines after end of function or class
- [E226](https://www.flake8rules.com/rules/E226.html): Missing whitespace around arithmetic operator
We should still enable the rules below:
- [E731](https://www.flake8rules.com/rules/E731.html): Do not assign a lambda expression, use a def
- [E741](https://www.flake8rules.com/rules/E741.html): Do not use variables named 'I', 'O', or 'l'
- [W503](https://www.flake8rules.com/rules/W503.html): Line break occurred before a binary operator
- [W504](https://www.flake8rules.com/rules/W504.html): Line break occurred after a binary operator
These rules are not enabled in this PR because:
- There are too many instances to fix. Maybe it's better to separate PRs
- Some required code changes.
- W503 and W504 are sort of conflicted. We should investigate a bit more.
### Why are the changes needed?
To comply PEP 8.
### Does this PR introduce _any_ user-facing change?
No, dev-only.
### How was this patch tested?
I manually tested it Python linter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #35126: [SPARK-37836][PYTHON][INFRA] Enable F841, E722, E305 and E226 for PEP 8 compliance
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35126:
URL: https://github.com/apache/spark/pull/35126#issuecomment-1007186286
Either one can be merged first, and the other one might have to be rebased. But there won't be real complicated conflicts. Just checked the diff by manually cherry-picking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #35126: [SPARK-37836][PYTHON][INFRA] Enable F841, E722, E305 and E226 for PEP 8 compliance
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35126:
URL: https://github.com/apache/spark/pull/35126#issuecomment-1007186286
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #35126: [SPARK-37836][PYTHON][INFRA] Enable F841, E722, E305 and E226 for PEP 8 compliance
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35126:
URL: https://github.com/apache/spark/pull/35126#issuecomment-1007168087
cc @zero323, @itholic, @ueshin, @xinrong-databricks FYI. I enabled rules that sort of need not so much changes, and that looks strait forward to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35126: [SPARK-37836][PYTHON][INFRA] Enable F841, E722, E305 and E226 for PEP 8 compliance
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35126:
URL: https://github.com/apache/spark/pull/35126#discussion_r780048734
##########
File path: python/pyspark/tests/test_readwrite.py
##########
@@ -231,9 +231,10 @@ def test_newhadoop(self):
)
self.assertEqual(result, data)
- fmt = "org.apache.hadoop.mapreduce.lib.output.SequenceFileOutputFormat"
Review comment:
this is technically a followup of https://github.com/apache/spark/pull/35126 but just decided to piggy-back here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #35126: [SPARK-37836][PYTHON][INFRA] Enable F841, E722, E305 and E226 for PEP 8 compliance
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35126:
URL: https://github.com/apache/spark/pull/35126#issuecomment-1007461103
Merged to master. Thanks guys!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35126: [SPARK-37836][PYTHON][INFRA] Enable F841, E722, E305 and E226 for PEP 8 compliance
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35126:
URL: https://github.com/apache/spark/pull/35126#discussion_r780048734
##########
File path: python/pyspark/tests/test_readwrite.py
##########
@@ -231,9 +231,10 @@ def test_newhadoop(self):
)
self.assertEqual(result, data)
- fmt = "org.apache.hadoop.mapreduce.lib.output.SequenceFileOutputFormat"
Review comment:
this is technically a followup of https://github.com/apache/spark/pull/35126 but just decided to piggy-back here since im touching arbitrary codes here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35126: [SPARK-37836][PYTHON][INFRA] Enable F841, E722, E305 and E226 for PEP 8 compliance
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35126:
URL: https://github.com/apache/spark/pull/35126#discussion_r780048412
##########
File path: dev/.gitignore
##########
@@ -1,2 +0,0 @@
-pep8*.py
Review comment:
We don't need it anymore after https://github.com/apache/spark/pull/34655.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zero323 commented on pull request #35126: [SPARK-37836][PYTHON][INFRA] Enable F841, E722, E305 and E226 for PEP 8 compliance
Posted by GitBox <gi...@apache.org>.
zero323 commented on pull request #35126:
URL: https://github.com/apache/spark/pull/35126#issuecomment-1007350779
E305, E226 should be covered by black, if enabled for particular path, but there is no harm in adding them.
Personally, I have mixed feelings about E722 ‒ using `BaseException` or `Exception` doesn't really seem to add anything. But I guess there is no harm in that either :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #35126: [SPARK-37836][PYTHON][INFRA] Enable F841, E722, E305 and E226 for PEP 8 compliance
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #35126:
URL: https://github.com/apache/spark/pull/35126
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #35126: [SPARK-37836][PYTHON][INFRA] Enable F841, E722, E305 and E226 for PEP 8 compliance
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #35126:
URL: https://github.com/apache/spark/pull/35126
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35126: [SPARK-37836][PYTHON][INFRA] Enable F841, E722, E305 and E226 for PEP 8 compliance
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35126:
URL: https://github.com/apache/spark/pull/35126#discussion_r780048734
##########
File path: python/pyspark/tests/test_readwrite.py
##########
@@ -231,9 +231,10 @@ def test_newhadoop(self):
)
self.assertEqual(result, data)
- fmt = "org.apache.hadoop.mapreduce.lib.output.SequenceFileOutputFormat"
Review comment:
this is technically a followup of https://github.com/apache/spark/pull/35126 but just decided to piggy-back here since im touching arbitrary codes here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zero323 commented on pull request #35126: [SPARK-37836][PYTHON][INFRA] Enable F841, E722, E305 and E226 for PEP 8 compliance
Posted by GitBox <gi...@apache.org>.
zero323 commented on pull request #35126:
URL: https://github.com/apache/spark/pull/35126#issuecomment-1007350779
E305, E226 should be covered by black, if enabled for particular path, but there is no harm in adding them.
Personally, I have mixed feelings about E722 ‒ using `BaseException` or `Exception` doesn't really seem to add anything. But I guess there is no harm in that either :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org