-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat(ci): add pre-commit
#184
Conversation
…neccessary hooks.
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.
Hey @JaeAeich - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
=======================================
Coverage 98.21% 98.21%
=======================================
Files 8 8
Lines 559 559
=======================================
Hits 549 549
Misses 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks @JaeAeich. I'm actually not so sure about having different checks for the pre-commit hook and the CI. Either we really care about things (and then we should enforce them, through the CI), or we don't (then why bother people at all?). Which is why I never ran with committing pre-commit checks at all and just do them for myself to save time on the overhead of CI checks. Thus, I would actually prefer to match the actual requirements across CI and hooks (minus long-running tests or tests that are difficult to run local). Would we really want to rely on users setting up the hooks to avoid spelling mistakes (which are hard to find even during reviews)? Btw, given our work mode where we squash merge, I think using a pre-push hook would be more appropriate. We allow feature branches to be messy, so no need to raise the bar for people to commit. For example, I sometimes commit and don't push when I feel I have reached a milestone, either because I feel that it's a commit that what really stand on its own, or just because I want or need to store my current work to check out some other branch. Sure, I could just skip the hook, but given that commits on feature branches aren't really the units we care about, I think the more natural timepoint to run checks in this flow would be prior to pushin. |
@uniqueg Yeah makes sense (thats what happened in cloud-component lol, thats why I wrote the PS in desc), on that note I have remove pre-commit as a git hook but left it in the CI, as |
Let's leave it open for now and see which of these checks might be worth including in the CI, because I think there are certainly some. |
Closing for now - can still be used as reference for CI future updates |
Add precommit, initialize it with necessary checks to keep code change abide by a standard.
Please look at
.pre-commit.config
for all the hooks that are added.For reviewer's sake, listing some config that have changed alot of files:
Note: This introduces a discrepancy in the checks between CI and pre-commit, it won't be hard (if someone wanted to) bypass precommit checks and push the code changes. Given adding all the hooks to CI as well might slow CI down, I thinks this discrepancy is the sweet spot. CI has checks for very absolutely necessary checks and pre-commit has some necessary and many nice-to-have checks.
EDIT: THIS
pre-commit
IS JUST FOR CI, NOT ADDED AS Agit
HOOK.