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

Working whitespace check #838

Closed
wants to merge 1 commit into from
Closed

Conversation

nishnik
Copy link
Contributor

@nishnik nishnik commented Mar 1, 2016

I googled a lot about this issue.
As a good practice it is advised to use Git hooks.
The default pre-commit hook is sufficient for whitespace check.
We can write a bash or python script to activate pre-commit hook (It just needs mv .git/hooks/pre-commit.sample .git/hooks/pre-commit). Need suggestions on that!

And adding this: git diff HEAD^1 --check in travis will serve our purpose. As it will exit the travis build if it finds trailing whitespace in recently modified code.

Instance where travis build fails due to trailing whitespace.
Instance when it runs fine when there aren't any whitespace. One job fails because piranha has been updated. Unrelated to this PR.

@abhinavagarwal07
Copy link
Contributor

Seems good to me.

@abhinavagarwal07
Copy link
Contributor

@nishnik
Please merge with latest master to get tests working.

@nishnik
Copy link
Contributor Author

nishnik commented Mar 1, 2016

ping @isuruf @certik

exit -1;
fi
# TODO: Add similar grep checks for space after comma,, space after `if`, space between `)` and `{` also
git diff HEAD^1 --check #test for whitespaces
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the idea here?
remove the top commit and take a diff as it looks to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't remove the top commit. Just returns the diff at it, i.e. the changes introduced in previous commit.
And diff --check is to check for trailing whitespaces.

--check

Warn if changes introduce whitespace errors.
What are considered whitespace errors is controlled by core.whitespace configuration.
By default, trailing whitespaces (including lines that solely consist of whitespaces) and a space character that is immediately followed by a tab character inside the initial indent of the line are considered whitespace errors.
Exits with non-zero status if problems are found.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrect.

@Sumith1896
Copy link
Contributor

@nishnik This misses out on some cases. Say you have a set of 10 commits, this checks only the top two. If the whitespace was introduced in the initial commits then they are missed out.

@nishnik
Copy link
Contributor Author

nishnik commented Mar 3, 2016

Oh! My bad!
I will look into this.

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.

None yet

3 participants