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

Create optional eslint pre-commit hook #1472

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marcustyphoon
Copy link
Collaborator

@marcustyphoon marcustyphoon commented May 31, 2024

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.

@AprilSylph
Copy link
Owner

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

@marcustyphoon
Copy link
Collaborator Author

  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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants