naturalcrit / homebrewery

Create authentic looking D&D homebrews using only markdown
https://homebrewery.naturalcrit.com
MIT License
1.05k stars 322 forks source link

Add pre-commit hooks #3384

Open Gazook89 opened 4 months ago

Gazook89 commented 4 months ago

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:

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

5e-Cleric commented 4 months ago

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

calculuschild commented 4 months ago

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 commented 4 months ago

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 commented 1 month ago

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 commented 1 month ago

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