XKit-Rewritten icon indicating copy to clipboard operation
XKit-Rewritten copied to clipboard

chore: Create optional eslint pre-commit hook

Open marcustyphoon opened this issue 1 year ago • 2 comments

Description

Something I was playing around with in another repository; ~~I have no experience using pre-commit lint hooks personally yet so I don't know if I find them helpful or clunky.~~ Edit: I've been running #1490 for a while now. It's nice!

This allows developers to—optionally—install a pre-commit hook that runs eslint locally ~on staged files~ before they're committed, preventing the commit if they are not in the semistandard style. This is nice for avoiding getting yelled at by the CI action.

(Traditional husky installs include a "prepare": "husky" package script, ensuring that all developers have the hook enabled. This does not.)

Testing steps

  • As per the readme, run npm run enable-hooks.
  • Make a change with a lint error (let a;), attempt to commit it, and confirm that the developer is informed about the error and must fix it before continuing.
  • Make a change with no lint error and observe the small added delay when committing it.
  • Make a change with a lint error (let a;) on a different branch and confirm that no hooks run (I chose husky primarily because it does not break in this scenario).
  • Run npm run disable-hooks, make a change with a lint error (let a;), attempt to commit it, and confirm that it is no longer prevented.

marcustyphoon avatar May 31 '24 08:05 marcustyphoon

Questions:

  1. What's the rationale behind making it optional? I feel like it would be a smoother experience to install the hook automatically.
  2. Why not do the full lint with npm test? web-ext lint errors are errors on CI, so...
  3. Could this be a precursor to us switching to the actual semistandard package for true autoformatting? :D

AprilSylph avatar Jul 09 '24 21:07 AprilSylph

  1. I know some developers don't like commit hooks! For example, the MV3 branch would have been impossible to work on in advance of web-ext being updated without bypassing hooks on each commit, and my setup doesn't have an easy way to bypass. I've seen arguments about whether they discourage frequent committing, as well, given that you still have tests run on push via CI. Didn't know what your opinion was on 'em.
  2. Fair; definitely an option! (I do wish it was faster. On another repo I use concurrently for multiple tasks; wouldn't be hard to set up here, though I'm not sure how well modified cli output in hooks works with some git setups.)
  3. I don't know of a difference between using the semistandard package and just running eslint!

marcustyphoon avatar Jul 09 '24 22:07 marcustyphoon