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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tools: Added git client hooks #1537

Merged
merged 4 commits into from Apr 13, 2019

Conversation

Projects
None yet
3 participants
@gverni
Copy link
Member

commented Apr 12, 2019

I've been playing with git client hooks lately and I feel they are a pretty powerful to make sure some policies are reinforced before something is committed. The first thing I thought about is to run linter in the pre-commit phase. This hook will run the lint task before a commit is happening. If the linter exits with error then the commit is aborted.

I know that we all run linter before we commit, and that git commit --amend is our best friend if we forget something 馃. But on the other hand I feel this is another automation tool that can save some time for us and make sure that who contribute to this project is following proper guidelines.

Also note that you can always bypass the check using git commit --no-verify.

To test this feature:

  • Clone Sweetalert2
  • Checkout this branch (feat/git-client-hooks)
  • Run npm or yarn install. At the end of the install procedure you should see:
> sweetalert2@8.8.3 postinstall 
> gulp install-git-client-hooks
  • Change one of the file adding for example a console.log() statement
  • Commit your changes. You should see
 git commit -am "Should be rejected"
(node:26515) ExperimentalWarning: The fs.promises API is experimental
[10:28:04] Using gulpfile ~/tmp/swal2/gulpfile.js
[10:28:04] Starting 'lint'...
...
  9:1  error  Unexpected console statement  no-console

And the commit is aborted (you can check the git log to make sure it's not there)

This PR is not complete. TODO list:

  • Tested on macOS with yarn and git version 2.6.4
  • Tested on macOS with npm and git version 2.6.4
  • Tested on win10 with npm and git version 2.18.0
  • Tested on win10 with yarn and git version 2.18.0
  • Tested on ubuntu 16.04 with npm and git version 2.7.4
  • Tested on ubuntu 16.04 with yarn and git version 2.7.4
  • Remove the install hook task if executed in TravisCI and other CI / CD tools (I'm doing this PR to check what happens)
  • Add uninstall hooks task
  • Get the agreement from the maintainer and collaborators 馃槃

Note: I decided to install the hook(s) in the .git/hooks/ folder instead of using the core.hooksPath configuration. The reason is that this configuration is supported only by Git version >=2.9

@limonte
Copy link
Member

left a comment

I personally hate when git commit takes a couple of seconds and will rm -rf .git/hooks right away :D

That's why I'd suggest adding the uninstall-git-client-hooks npm script.

Otherwise, I think that's a good improvement which will enforce new contributors to commit the correctly formatted code.

Show resolved Hide resolved package.json Outdated
@limonte

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

I'm using the latest Git version (2.21.0) and can confirm that everything works nicely with Yarn and Ubuntu 18.04

@limonte limonte force-pushed the master branch from d050c8c to fbd02ad Apr 12, 2019

@limonte limonte force-pushed the feat/git-client-hooks branch 2 times, most recently from 4415094 to 55cf759 Apr 13, 2019

@limonte limonte marked this pull request as ready for review Apr 13, 2019

@limonte limonte changed the title tools: Added git client hooks [WIP] tools: Added git client hooks Apr 13, 2019

@limonte

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

I made this a PR to run Travis builds, I'll check everything on Windows machine later today.

@coveralls

This comment has been minimized.

Copy link

commented Apr 13, 2019

Pull Request Test Coverage Report for Build 4867

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 91.176%

Totals Coverage Status
Change from base Build 4866: 0.0%
Covered Lines: 1233
Relevant Lines: 1300

馃挍 - Coveralls
@gverni

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

Thank you so much @limonte. I'll check yarn on linux later as well (though I'm expecting it to work fine).

@gverni

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

Tested on win10 (with both npm and yarn) 馃憤

@limonte limonte changed the title [WIP] tools: Added git client hooks Tools: Added git client hooks Apr 13, 2019

gverni and others added some commits Apr 11, 2019

Added gulp task to install Git client hooks
The hook's scripts are stored in folder .githooks. The task is copying
the content of this folder to .git/hooks/
Added postinstall script
The post install script is executing the install-git-client-hooks
gulp task

@limonte limonte force-pushed the feat/git-client-hooks branch from 55cf759 to 3973715 Apr 13, 2019

@codeclimate

This comment has been minimized.

Copy link

commented Apr 13, 2019

Code Climate has analyzed commit 3973715 and detected 0 issues on this pull request.

View more on Code Climate.

@limonte limonte merged commit 14c6d22 into master Apr 13, 2019

5 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
WIP Ready for review
Details
bundlesize dist/sweetalert2.all.min.js: 15.35KB < maxSize 16KB (gzip)(same as master)
Details
codeclimate All good!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
deploy/netlify Deploy preview ready!
Details

@limonte limonte deleted the feat/git-client-hooks branch Apr 13, 2019

@limonte

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

馃帀 This PR is included in version 8.8.6 馃帀

The release is available on:

Your semantic-release bot 馃摝馃殌

@limonte limonte added the released label Apr 21, 2019

limonte added a commit that referenced this pull request Apr 21, 2019

limonte added a commit that referenced this pull request Apr 21, 2019

limonte pushed a commit that referenced this pull request Apr 21, 2019

chore(release): 8.8.7 [skip ci]
## [8.8.7](v8.8.6...v8.8.7) (2019-04-21)

### Bug Fixes

* revert "chore(tools): git hooks for running linters before commit ([#1537](#1537))" ([#1559](#1559)) ([d22b234](d22b234))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.