[GitHub] [spark] HyukjinKwon commented on a change in pull request #27912: [SPARK-31155] Enable pydocstyle tests
HyukjinKwon commented on a change in pull request #27912: [SPARK-31155] Enable pydocstyle tests URL: https://github.com/apache/spark/pull/27912#discussion_r392638387 ## File path: dev/requirements.txt ## @@ -1,5 +1,8 @@ -flake8==3.5.0 +pycodestyle==2.5.0 Review comment: On the other hand, not pinning will trigger us to fix quickly to support latest versions. Pinning version will force developers to install different versions of libraries they already have. If something is found we can't easily fix, we could pin the version. If I am not completely wrong, using the latest version by default is the conventional approach in Python side (and R side - it reminds me of [`testthat` case](https://github.com/apache/spark/pull/27359)). This is the approach used in [pandas as an example](https://github.com/pandas-dev/pandas/blob/master/requirements-dev.txt). [I used this approach too](https://github.com/databricks/koalas/blob/master/requirements-dev.txt). I agree, in a way, it can make more sense to pin the version in particular when this is used for CI purpose or release docker image which are supposed to discard of after using once; however, I don't quite agree when this affects other developers' environments. cc @dongjoon-hyun. IIRC, I think we have related discussions at https://github.com/apache/spark/pull/26824. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #27912: [SPARK-31155] Enable pydocstyle tests
HyukjinKwon commented on a change in pull request #27912: [SPARK-31155] Enable pydocstyle tests URL: https://github.com/apache/spark/pull/27912#discussion_r392638919 ## File path: dev/tox.ini ## @@ -18,4 +18,4 @@ ignore=E226,E241,E305,E402,E722,E731,E741,W503,W504 max-line-length=100 exclude=cloudpickle.py,heapq3.py,shared.py,python/docs/conf.py,work/*/*.py,python/.eggs/*,dist/*,.git/* [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 +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,D415,D416,D417 Review comment: Also, I think we didn't mention that we comply [PEP-257](ttps://www.python.org/dev/peps/pep-0257/) yet anywhere. Of course, It's good to incrementally fix to comply PEP-257; however, it ignores too many rules currently. I wonder if we added this too early without a comprehensive investigation. There were several rules that looked counterintuitive to me, IIRC. cc @BryanCutler who, IIRC, also pointed this out. BTW, many rules were disabled due to our legacy Epydoc, which I managed to remove at https://github.com/apache/spark/commit/fe75ff8bea3330a10aba1a61f3aba42e541195a8. So, several rules might be able to enable back now. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #27912: [SPARK-31155] Enable pydocstyle tests
HyukjinKwon commented on a change in pull request #27912: [SPARK-31155] Enable pydocstyle tests URL: https://github.com/apache/spark/pull/27912#discussion_r392638919 ## File path: dev/tox.ini ## @@ -18,4 +18,4 @@ ignore=E226,E241,E305,E402,E722,E731,E741,W503,W504 max-line-length=100 exclude=cloudpickle.py,heapq3.py,shared.py,python/docs/conf.py,work/*/*.py,python/.eggs/*,dist/*,.git/* [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 +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,D415,D416,D417 Review comment: Also, I think we didn't mention that we comply [PEP-257](ttps://www.python.org/dev/peps/pep-0257/) yet anywhere. Of course, It's good to incrementally fix to comply PEP-257; however, it ignores too many rules currently. I wonder if we added this too early without a comprehensive investigation. There were several rules that looked counterintuitive to me, IIRC too. cc @BryanCutler who, IIRC, also pointed this out. BTW, many rules were disabled due to our legacy Epydoc, which I managed to remove at https://github.com/apache/spark/commit/fe75ff8bea3330a10aba1a61f3aba42e541195a8. So, several rules might be able to enable back now. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #27912: [SPARK-31155] Enable pydocstyle tests
HyukjinKwon commented on a change in pull request #27912: [SPARK-31155] Enable pydocstyle tests URL: https://github.com/apache/spark/pull/27912#discussion_r392638671 ## File path: dev/tox.ini ## @@ -18,4 +18,4 @@ ignore=E226,E241,E305,E402,E722,E731,E741,W503,W504 max-line-length=100 exclude=cloudpickle.py,heapq3.py,shared.py,python/docs/conf.py,work/*/*.py,python/.eggs/*,dist/*,.git/* [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 +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,D415,D416,D417 Review comment: I don't think it's intentional. Actually, I have been considering removing `pydocstyle` out which doesn't verify things very useful. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #27912: [SPARK-31155] Enable pydocstyle tests
HyukjinKwon commented on a change in pull request #27912: [SPARK-31155] Enable pydocstyle tests URL: https://github.com/apache/spark/pull/27912#discussion_r392638387 ## File path: dev/requirements.txt ## @@ -1,5 +1,8 @@ -flake8==3.5.0 +pycodestyle==2.5.0 Review comment: On the other hand, not pinning will trigger us to fix quickly to support latest versions. Pinning version will force users to install different versions of libraries they already have. If something is found we can't easily fix, we could pin the version. If I am not completely wrong, using the latest version by default is the conventional approach in Python side (and R side - it reminds me of [`testthat` case](https://github.com/apache/spark/pull/27359)). This is the approach used in [pandas as an example](https://github.com/pandas-dev/pandas/blob/master/requirements-dev.txt). [I used this approach too](https://github.com/databricks/koalas/blob/master/requirements-dev.txt). I agree, in a way, it can make more sense to pin the version in particular when this is used for CI purpose or release docker image which are supposed to discard of after using once; however, I don't quite agree when this affects other developers' environments. cc @dongjoon-hyun. IIRC, I think we have related discussions at https://github.com/apache/spark/pull/26824. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
