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

*: added pre-commit hook. #3112

Merged
merged 8 commits into from
Apr 24, 2017
Merged

Conversation

zhexuany
Copy link
Contributor

After this, future commited code has to satisfy the rule we specify in
pre-commit.

@coocood
Copy link
Member

coocood commented Apr 22, 2017

Good idea!
pre-commit hook is a great tool.
Is this script referenced from other go projects?

@zhexuany
Copy link
Contributor Author

@coocood Yes. It comes from K8s. I will add credits later.

@zhexuany zhexuany force-pushed the added_pre_commit_hook branch from 0757b8e to 591b272 Compare April 22, 2017 07:10
@ngaut
Copy link
Member

ngaut commented Apr 22, 2017

LGTM

@zhexuany zhexuany force-pushed the added_pre_commit_hook branch 2 times, most recently from 591b272 to 7d03940 Compare April 22, 2017 14:53
@shenli
Copy link
Member

shenli commented Apr 23, 2017

LGTM
Please add credits or license for the scripts.


echo -ne "Checking for files that need goword... "
files_need_goword=()
files=($(git diff --cached --name-only --diff-filter ACM | grep "\.go" | grep -v -e "^_vendor"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is --diff-filter ACM?

Copy link

@iamzhout iamzhout Apr 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hanfei1991 https://git-scm.com/docs/git-diff#git-diff---diff-filterACDMRTUXB82308203

--diff-filter=[(A|C|D|M|R|T|U|X|B)…​[*]]
Select only files that are Added (A), Copied (C), Deleted (D), Modified (M), Renamed (R), have their type (i.e. regular file, symlink, submodule, …​) changed (T), are Unmerged (U), are Unknown (X), or have had their pairing Broken (B). Any combination of the filter characters (including none) can be used. When * (All-or-none) is added to the combination, all paths are selected if there is any file that matches other criteria in the comparison; if there is no file that matches other criteria, nothing is selected.

Also, these upper-case letters can be downcased to exclude. E.g. --diff-filter=ad excludes added and deleted paths.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got

@zhexuany zhexuany force-pushed the added_pre_commit_hook branch from e60560e to b0e26a5 Compare April 24, 2017 11:27
@zhexuany
Copy link
Contributor Author

@shenli PTAL

@zhexuany zhexuany force-pushed the added_pre_commit_hook branch from bde2344 to 9b3d99e Compare April 24, 2017 13:59
Makefile Outdated

default: server buildsucc
default: server buildsucc precommithook
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not add it in default.

@@ -23,7 +23,7 @@ import (

// extractCorColumnsBySchema only extracts the correlated columns that match the outer plan's schema.
// e.g. If the correlated columns from inner plan are [t1.a, t2.a, t3.a] and outer plan's schema is [t2.a, t2.b, t2.c],
// only [t2.a] is treated as this apply's correlated column.
// only [t2.a] is treated as this applies correlated column.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated change.

Makefile Outdated
@@ -140,3 +140,7 @@ endif
glide vc --only-code --no-tests
mkdir -p _vendor
mv vendor _vendor/src

precommithook:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As comments are added in the pre-commit file, this is redundant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One time job should not be added to Makefile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

hooks/pre-commit Outdated
files_need_goword=()
files=($(git diff --cached --name-only --diff-filter ACM | grep "\.go" | grep -v -e "^_vendor"))
for file in "${files[@]}"; do
# Check for files that fail gofmt.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goword

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

Copy link
Member

@coocood coocood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@coocood coocood merged commit 2fcd97f into pingcap:master Apr 24, 2017
@zhexuany zhexuany deleted the added_pre_commit_hook branch April 24, 2017 15:21
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.

6 participants