hterik commented on a change in pull request #19155:
URL: https://github.com/apache/airflow/pull/19155#discussion_r735274303
##########
File path: clients/gen/common.sh
##########
@@ -71,6 +71,8 @@ function run_pre_commit {
# prepend license headers
pre-commit run --all-files || true
+ echo "Output above will state Failed: 'Some sources were modified by the
hook' - This is expected as the hook is used to add the licences."
+ pre-commit run --all-files
Review comment:
The `|| true` will ignore all types of errors, both the expected errors
from adding licenses, and other unexpected errors that pre-commit can't autofix.
For example, first time i ran this it failed on not having pre-commit
installed at all, but script still ran to completion as though it succeeded.
I don't have enough experience with pre-commit or the types of linters that
are installed in this project to say if this is desired or not. I see
https://pre-commit.com/#cli has a few different exit-codes to help but again, i
don't have experience with this to tell what they mean by expected vs
unexpected error in this context.
Maybe ignoring this error with || true isn't a big deal as it would be
caught by CI runner anyway?
I'd be happy to exclude this change from this MR just to get the relevant
fixes merged. This is more of a parallel improvement.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
