-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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 hook to run frontend linters #7075
Conversation
How does this interact with existing hooks? |
It will override it as it sets the hooks path config option and sets it to Basically the pre-commit hook will be managed in the project and I'm not sure if there's a way for you to add your own one on top other than getting the project to accept it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last time I worked with this kind of linter at commit time, it was not an ideal experience for the developers. Your code is modified after you have prepared everything, which is surprising sometimes.
With the linters being supported by most of the IDEs, the native "format" shortcut should take that into consideration. ("should" as Jenkins has multiple different linters)
For folks working on their enterprise laptops, do you have any pre-commit hooks policy that could be impacted by this kind of change?
Do you know what is the time it takes to lint a commit ? I expect if you are working on pure Java classes, the impact should be close to 0, but I prefer to ask.
It was a pain in a previous project (badly configured) ;-)
@@ -0,0 +1,5 @@ | |||
#!/usr/bin/env sh | |||
. "$(dirname -- "$0")/_/husky.sh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally despite the shell script, it's supported on Windows (https://typicode.github.io/husky/#/?id=does-it-work-on-windows)
Now, it could be interesting to get some Windows users to ensure it's really working.
On my own system it was working, but perhaps I have too many stuff installed that are necessary for Jenkins development.
Close to 0, not really noticeable as it only works on staged files rather than doing something like running a whole maven build |
Please take a moment and address the merge conflicts of your pull request. Thanks! |
-0 from me, I hate pre-commit hooks and would much rather run a Maven command to fix up formatting when I want rather than at Git commit time. If this is accepted, the first thing I will do will be to disable it on my local machine. |
Could you explain why? / have you tried this out to get a feel for it to see if it would impact your worflow?
sure you can just unset the config item it adds if you want to do that |
This is just personal preference. I am not against others using what works for them, but this does not work for me. I don't like it when my Git operations take anything more than 30 or so milliseconds, and I don't like it when they do anything other than source control operations. When I write code I create a lot of "private" commits that mean nothing to anyone other than me, squashing them together into something logically coherent before publishing the result, so my workflow relies on commits being fast and surprise-free. I appreciate that others have different workflows, which is why I started this comment by stating that it is a matter of personal preference. As long as I can opt-out I am OK. |
Please take a moment and address the merge conflicts of your pull request. Thanks! |
Please take a moment and address the merge conflicts of your pull request. Thanks! |
Co-authored-by: Jesse Glick <[email protected]>
Please take a moment and address the merge conflicts of your pull request. Thanks! |
Seems to be little appetite for this so closing |
I've noticed a few builds failing from linting related errors.
This should automatically fix those that are auto-fixable.
Lint errors that aren't auto-fixable (some eslint and stylelint ones) will fail the commit.
The git hook will be automatically installed via husky.
It can be bypassed by running
git commit --no-verify
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Proposed changelog entries
section only if there are breaking changes or other changes which may require extra steps from users during the upgrade@Restricted
or have@since TODO
Javadoc, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
if applicable.eval
to ease future introduction of Content-Security-Policy directives (see documentation on jenkins.io).Maintainer checklist
Before the changes are marked as
ready-for-merge
:Proposed changelog entries
are accurate, human-readable, and in the imperative moodupgrade-guide-needed
label is set and there is aProposed upgrade guidelines
section in the PR title. (example)lts-candidate
to be considered (see query).