Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[v2] Add pre-commit and CI static checks #1025
[v2] Add pre-commit and CI static checks #1025
Changes from 6 commits
916517f
381fa7d
8be7c75
3abbd50
dfbd618
7acc1cf
c1bf8d2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great, but how does this work? Should add some docs on this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that's enough to google
doctoc
to find out more information. Also not sure where to put such docsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. And wit pre-commits you basically do not have to worry about it - just install pre-commit and you forget about it..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a bit of an odd argument as that would mean we don't have to doc the pre-commit thingy but it's ok.
We tend to make it easy for folks to contribute so you cannot assume everybody has
pre-commit
installed, that's why we have a CI.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we have totally lightweight pre-commit setup we should probably encourage people to use them. It's good to teach people how to automate stuff and focus on the most important things like the code itself and documentation. However, that's only my personal opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's definitely the case and I agree, but we can't expect everyone to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have it integrated on the CI, so if someone does not use pre-commit locally, they will get a readable diff on the CI that will show what changes should be made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was my point on #1025 (comment) indeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Pre-commits are working like a charm on Apache Airflow for > 1.5 year now and we have plenty of pre-commits running/ You can see it working here for example: https://github.com/apache/airflow/runs/1026196376?check_suite_focus=true#step:7:108 - it integrates very well with CI. you simply run
pre-commit run --all-files
in the CI and you get the very same checks executed in CI as locally.And as @mik-laj mentioned - with
--show-diff-on-failure
flag it will even show you what modification would be made if you run pre-commit locally. In case it fails in CI it will even tell you what to do 'pip install pre-commit' and thenpre-commit run <id of pre-commit that failed>
. But you still can fix it manually if you prefer (though a lot of pre-commit checks can fix things for you automatically (like end-of-file checking, sorting import in python etc.) and then the only thing you have to do you have to commit modified files. And you have literally hundreds of hooks that you can start using.So you do not have to even install it as pre-commit (though it is super-easy
pre-commit install
will do the job after installit it with pip. The nice thing about it is that you simply forget about it once you install - it will automatically run all the precommits that were added by others since and you will actually fix the problems even before others see them.I do not see any drawbacks of using pre-commit to be honest if your project's goal is to keep some consistency, it's the perfect tool for the job IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't get me wrong, I'm not saying it's a bad thing or so; I was just saying that it's fine and the CI will cover it but we shouldn't force people to use it.
I'm not saying you are, it was just a general remark :) This escalated quickly.