chore: add opt-in pre-commit lint --write hook#2475
Conversation
|
Thanks for the contribution! Before we can merge this, we need @zi0w to sign the Salesforce Inc. Contributor License Agreement. |
|
I’ve signed the Salesforce CLA. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2475 +/- ##
=======================================
Coverage 90.67% 90.67%
=======================================
Files 82 82
Lines 14994 14994
Branches 508 508
=======================================
Hits 13596 13596
Misses 1373 1373
Partials 25 25
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
hi, @zi0w! Thank you so much for the contribution. I have run the workflow run for this PR, and it looks like there are a few failing tests across Node versions 18, 20, and 22 for the Also, please confirm that you have signed the CLA using the same email and GitHub account as the one you've submitted the PR for - it looks like that check failed as well, so just want to verify whether I need to close or re-open the PR if this is reporting a false result. |
| @@ -0,0 +1,54 @@ | |||
| #!/bin/sh | |||
| set -e | |||
There was a problem hiding this comment.
this implementation is very complex - is there a way we could simplify this?
|
Hi @hello-ashleyintech — thanks for running the workflows! CLA: I already signed the Salesforce CLA using the same email/account as this PR, but it still shows cla:missing. Do you know if there’s a way for me to retry or trigger a re-check, or anything else I should do to resolve it? CI failures: I’m looking into the Windows failures (Node 18/20/22) for logger, web-api, and cli-test now. I’ll review the logs and push an update once I find the cause. Hook complexity: Agreed it’s a bit heavy. I’ll simplify the hook logic while keeping the same behavior (only run for staged packages/ changes + auto-fix + re-stage) and include that in the next commit. |
|
Hi, @hello-ashleyintech! Quick update: CLA: Looks like the CLA check is now passing after the latest commit — thank you! Hook complexity: I simplified the pre-commit hook to reduce parsing/branching while keeping the behavior the same. It derives the touched packages/ from staged files, runs lint:fix when available (otherwise falls back to lint -- --fix), and re-stages any auto-fixes. I also adjusted it so the lint output is visible during commits to make local verification/debugging easier. CI failures: The Windows jobs were failing very quickly (~a few seconds) with the same message: Thanks again for taking a look! |
|
@zi0w I have closed and re-opened the PR to bump the CLA check, and it looks like it's now working. I will now approve the workflows to run! |
|
👋 #2478 might make adjacent changes related to linting in a monorepo. FWIW I'm not familiar with pre lint hooks- |
|
Hi @zimeg — thanks for the heads-up! Yep, this PR is an opt-in pre-commit hook that runs lint --write (via each package’s lint:fix / lint -- --fix) only for staged changes, and re-stages any auto-fixes. It shouldn’t change CI behavior directly, but I agree it’s adjacent to monorepo linting/workspace changes like #2478. If there’s anything you’d like me to align with (e.g., preferred package detection or how lint commands should be invoked in the monorepo), I’m happy to adjust. |
|
Hi @hello-ashleyintech — CLA check is now passing and all CI checks are green. I also simplified the opt-in pre-commit hook while keeping the same behavior (lint:fix → fallback to lint -- --fix, then re-stage). Thanks again! |
|
Hi @hello-ashleyintech — just a gentle ping on this when you have a moment. Since my last update, CLA is passing and all checks are green. Would you be able to take a quick look / leave a review when you get a chance? Thanks so much! |
|
hi @zi0w, apologies for the delay! It looks like this branch has a merge conflict, could you resolve it? I discussed this solution with the team and we had a couple feedback items:
let me know if you have any questions! |
c0d8de3 to
b3cea66
Compare
|
|
Hi @hello-ashleyintech — quick note: I’m still looking into this, but I’m about to log off for today. I’ll follow up with a quick update tomorrow or the day after. Thanks! |
|
Hi @hello-ashleyintech! I updated the opt-in pre-commit hook to a minimal version that runs |
zimeg
left a comment
There was a problem hiding this comment.
@zi0w Ahha since learning about this I've found I want this check more and more... CI keeps erroring for me! 😉
This is in a solid place but I'm leaving a few fast suggestions. Thanks so much for keeping this branch current as we're making changes alongside!
.githooks/pre-commit
Outdated
| #!/bin/sh | ||
| set -e | ||
| cd "$(git rev-parse --show-toplevel)" || exit 1 | ||
| npm run lint No newline at end of file |
There was a problem hiding this comment.
👾 quibble: Can we end this file with a newline? It might not be linted outside of packages I believe...
.githooks/pre-commit
Outdated
| @@ -0,0 +1,4 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
| #!/bin/sh | |
| #!/usr/bin/env bash |
🌲 suggestion: This might be a more standard path to use across setups!
.github/maintainers_guide.md
Outdated
| #### Pre-commit lint hook (optional) | ||
| We provide an opt-in Git hook that runs `npm run lint` from the repository root before a commit is created. | ||
|
|
||
| Enable it once per clone: | ||
|
|
||
| ```sh | ||
| git config core.hooksPath .githooks | ||
| ``` | ||
|
|
||
| Disable it: | ||
|
|
||
| ```sh | ||
| git config --unset core.hooksPath | ||
| ``` | ||
|
|
||
| Notes: | ||
| - The hook runs `npm run lint` from the repository root. | ||
| - You can skip it with `git commit --no-verify` if needed. | ||
|
|
There was a problem hiding this comment.
📚 suggestion: Let's add this as a new task before the "testing and linting" section! Perhaps as:
📠 Project Setup
Clone this repository with the following command:
git clone https://github.com/slackapi/node-slack-sdk
With more details on hooks in following sentences?
There was a problem hiding this comment.
👁️🗨️ question: Would moving this to the paths following be better for organization or is ".githooks" standard for discovering these scripts?
scripts/hooks/...
|
Hi, @zimeg! Thanks for the quick suggestions!
I kept the directory as |
Summary
Fixes #2034
Adds an opt-in Git pre-commit hook that runs lint auto-fix (
lint:fixorlint -- --fix) for only the packages touched in staged changes.Also documents how to enable it via
core.hooksPathin the maintainer’s guide.Requirements (place an
xin each[ ])Changes
.githooks/pre-commit(opt-in): runs package-level lint auto-fix for staged changes only.github/maintainers_guide.md: document how to enable the hookTest plan
git config core.hooksPath .githooksgit commitand confirm the hook runs lint auto-fix and the commit succeeds.