Skip to content

Conversation

@bhamodi
Copy link
Contributor

@bhamodi bhamodi commented Jun 1, 2015

@jordangarcia - accomplished this using the grunt githooks module.

Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be grunt eslint since using just eslint requires eslint to be globally installed on the users machine.

@jordangarcia
Copy link
Contributor

I checked out this branch and can't commit since there are style failures. This githook should only be created once its clear to do a merge. We wouldn't want any contributor unable to commit becuase of existing style errors.

@bhamodi
Copy link
Contributor Author

bhamodi commented Jun 1, 2015

Yup, just saw that. There's 10 lint warnings left to address. I'll send a PR for that before this gets merged in.

@bhamodi
Copy link
Contributor Author

bhamodi commented Jun 1, 2015

By the way, it does allow you to commit if there are only lint warnings and no lint errors. Not sure why you weren't able to send a commit? There's only 10 lint warnings, no errors.

@jordangarcia
Copy link
Contributor

ah i must have not noticed that, since i wasn't commiting any changed files.

@bhamodi
Copy link
Contributor Author

bhamodi commented Jun 1, 2015

It's all good. Either way, I'm working on sending a PR for the last 10 warnings. By the way, on the topic of warnings and errors - we might want to revise what we consider to be an error/warning. I say this because only errors will actually stop a commit from happening and force a user to fix the styling. A warning will only display the warnings but still allow the commit through. So if there's things for sure that we don't want committed, we'll have to change the value to 2 in .eslintrc.

@jordangarcia
Copy link
Contributor

hmm i dont really like making everything errors, usually warnings are style concerns and errors are actual errors that will affect the runtime.

Is there a way to make grunt eslint fail on warning.

The way githooks work, is if the pre-commit process returns a non-zero exit code then it wont let you commit. So it seems like we need a way to have grunt eslint return the appropriate exit code to prevent commiting.

@bhamodi
Copy link
Contributor Author

bhamodi commented Jun 1, 2015

Ya, I looked into making it fail on warnings but the issue is that the grunt eslint command returns an exit code of 0 when only warnings exist. I'm not sure how to override the eslint module to return a non-zeo exit code.

@bhamodi
Copy link
Contributor Author

bhamodi commented Jun 2, 2015

Ok @jordangarcia - just sent in #81 to resolve the last 10 lint warnings.

@jordangarcia
Copy link
Contributor

this lgtm

bhamodi added a commit that referenced this pull request Jun 2, 2015
Added pre-commit hook to enforce lint style
@bhamodi bhamodi merged commit 89cd28f into optimizely:master Jun 2, 2015
@bhamodi bhamodi mentioned this pull request Jun 2, 2015
3 tasks
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.

2 participants