KevinGG commented on pull request #12444: URL: https://github.com/apache/beam/pull/12444#issuecomment-672309994
> Few things regarding this change > > 1. Isn't the spotless pre-commit right place to extend with such checks? Many developers probably already added that to their pre-commit hooks so the introduction of this check will be almost unnoticeable. It will be the best if the check is unnoticeable. This check can also get triggered when a commit only changes some markdown or build.gradle files in the repo. It's not needed if the commit just changes code since different linters in those programming languages already covers the whitespace issues in code files. > 2. Seems like it's using python2.7 virtualenv [1]: > > ``` > 00:00:40.125 Running virtualenv with interpreter /usr/bin/python2.7 > 00:00:40.425 DEPRECATION: Python 2.7 reached the end of its life on January 1st, 2020. > ``` > > If possible, I would suggest avoiding that due to the end of life. > Changing it to use python3.8. > 1. The check is really "quiet" in terms of logging, in case of failure there are no details regarding the file/line and the exact issue. Check out my sample PR #12470 and the build output [2]. Would be helpful if it prints the issue similarly to the spottles e.g. [3]. > > [1] https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Commit/1/console > [2] https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Commit/3/console > [3] https://ci-beam.apache.org/job/beam_PreCommit_Spotless_Commit/10575/console Yes, it doesn't print much except `Running whitespacelint...` if it succeeds. But if it fails, it does tell you which line and what problem it is. The [2] actually fails because it couldn't find the precommit task: ` Task 'whitespacePreCommit' not found in root project 'beam'.` I'll modify the script to output more information. ---------------------------------------------------------------- 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: [email protected]
