Skip to content
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

Add pre-commit hooks #3384

Open
Gazook89 opened this issue Apr 1, 2024 · 5 comments
Open

Add pre-commit hooks #3384

Gazook89 opened this issue Apr 1, 2024 · 5 comments
Labels
cleanup Cleaning up code for legibility, style, ease-of-use, etc.

Comments

@Gazook89
Copy link
Collaborator

Gazook89 commented Apr 1, 2024

Your idea:

For the repository I was wondering if anyone is interested in pre-commit hooks (git) to run Stylelint and ESLint on their changed files either before each commit or before doing a push? This would be automatic, so no more manually running it. I am looking at using Husky to access the hooks, and then lint-staged npm module to run scripts only on the Staged files.

One bad thing about this is that a change on a single line will result in the whole file being linted-- since we have files that haven't been linted before we will be adding a git-blame layer on every line [that gets linted]. This might not be a big issue, though-- if you ever git-blame and see this being the case, you can just go back in commit history one step. Or, maybe this can help (though it's unclear to me if there is any value if looking at git-blame via another tool).

A question: Would you rather that the linters work on every commit, or only when pushing your commits?

More questions:
Would you rather:

  • Run the linters on every commit (pre-commit) and automatically apply fixes and proceed thru commit process, without notifying you of the linter changes?
  • Run the linters on every commit (pre-commit) but cancel the commit process and show you the errors in terminal (without fixing)?
  • Run the linters on every commit (pre-commit) and automically apply fixes but cancel the commit process so you can scan the result before trying again?
  • or the above, but all on pre-push?

Are there other scripts you'd like to run pre-commit or pre-push? Or even pre-merge?

@5e-Cleric
Copy link
Member

I have actually never run the linters, this looks like a good idea, we should do this.

@5e-Cleric 5e-Cleric added the cleanup Cleaning up code for legibility, style, ease-of-use, etc. label Apr 2, 2024
@calculuschild
Copy link
Member

I would need to think about which option I prefer, but, before this, we should run a linting pass over every file at least once (maybe group related files into different linting PRs) to avoid the mass-blame issue you mention.

I.e., start from a clean, fully-linted base so any future PRs using these hooks aren't full of unrelated spacing adjustments scattered around the file.

@Gazook89
Copy link
Collaborator Author

Gazook89 commented Apr 2, 2024

One recommendation I saw is to create a temporary github user for just this purpose, named something like "Lint-User" or something, to have it do a mass linting/formatting. Then be sure that that commit is pulled into every open branch before they merge back in. I think this likely applies to teams where people really care about how much of the project is "theirs", and maybe doesn't apply here. But it might still be useful when reviewing blame.

Or perhaps something could be configured to do everything we are talking about, but like this:

  1. commit actual changes
  2. run linter/formatters
  3. automatically create a second commit with just those changes (with a standard commit message like "apply lint & format")
  4. push

Though if we do that on every commit, it might be tedious to review the commits (you'll have ~2x as many commits).

@G-Ambatte
Copy link
Collaborator

On a similar note, today I stumbled across "format on save" and "lint fix on save"; it was certainly being discussed as if it is a standard option for code editors. I'm about to go experiment with my VSCode install - I'll add notes on anything useful that I learn.

@G-Ambatte
Copy link
Collaborator

I have the ESLint extension installed for VSCode, adding this to the settings.json file appears to lint every file when it is saved:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Cleaning up code for legibility, style, ease-of-use, etc.
Projects
None yet
Development

No branches or pull requests

4 participants