search

withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
45.47k stars 2.38k forks source link

💡 RFC: Git pre-commit hook for lint and code format. #713

Closed devsmitra closed 3 years ago

devsmitra commented 3 years ago

Background & Motivation

Can be used to lint commit messages, run tests, lint code, etc. at the time of commit or push. This can be done using CI as well but using hook developers will be informed before pushing their code to the git.

Proposed Solution

Possible solutions

I have already suggested a solution for this here https://github.com/snowpackjs/astro/pull/698

Alternatives considered

Risks, downsides, and/or tradeoffs

Open Questions

Detailed Design

No response

Help make it happen!

FredKSchott commented 3 years ago

Thanks for creating the issue! Copying my response from the PR here: I

personally don't love Husky, and would rather automate these things via CI instead outside of the PR flow on each contributors machine. For example, we automatically format the codebase after every commit, so there's no need for a husky format precommit check.

devsmitra commented 3 years ago

@FredKSchott I agree, We can do this using CI but if we use git hooks developer will know before commit/push. Instead of going and checking lint issue GitHub action or CI, It'll help us to fix common issues then and there.

jasikpark commented 3 years ago

@FredKSchott I agree, We can do this using CI but if we use git hooks developer will know before commit/push. Instead of going and checking lint issue GitHub action or CI, It'll help us to fix common issues then and there.

The prettier CI doesn't work as a lint though, it works as a codemod - every commit to main is formatted automatically just like if we had git hooks setup.

devsmitra commented 3 years ago

@jasikpark I think we can skip the format, but can we do it for lint checks. because I see some lint issue and warning is main. We avoid these issues in the future.

Just for reference

✖ 27 problems (4 errors, 23 warnings)
 1 error and 0 warnings potentially fixable with the `--fix` option.
jthegedus commented 3 years ago

You could probably just add eslint --fix to the auto format as well? Would fix a bunch of the issues.

jasikpark commented 3 years ago

It looks like #802 added running the lint commands to the CI (just the linting not fixing them), so hopefully we can just PR a commit that makes those improvements now that they're visible... while we're still figuring this out, want to submit a PR that does just that?

devsmitra commented 3 years ago

@jthegedus I have added eslint --fix in the pre-commit check.

@jasikpark I'll raise another PR with changes suggested by @jthegedus. If we agree to merge these changes

FredKSchott commented 3 years ago

I'd like to close this because I'm strongly -1 on adding husky at this stage in the project (for the reasons outlined above and in that PR).

If there's interest from other L2+ contributors I'm happy to reopen and discuss more, but otherwise I'd like to push us to improving CI to better surface these issues instead, and then revisit this once we've hit v1.0 release.