937431 - [Tooling] make jshint a requirement on pre-commit

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Corey Frang [:gnarf]]

Attachment: github PR

We are trying to move over to using jshint instead of gjslint, however if we never fix the backlog of errors currently in jshint, we will never be able to use it.

I propose we create a .jshintwhitelist and a custom reporter that can whitelist errors in files we haven't yet fixed to be jshint compatible.

The output from the `make hint` could then be a required green on travis as well, only files listed in .jshintwhitelist would then be allowed to have jshint errors.

The output from the pre-commit hook would show the jshint errors, but not stop the commit if they are whitelisted, and can gently encourage people to fix simple jshint errors and remove the file from the whitelist as well.

Comment 1

[James Lal [:lightsofapollo]]

Comment on attachment 830590 [details] [review]
github PR

I didn't look super closely at the code but I really like the concept and think this is probably the best way we can move from gjslint -> jshint. The important thing here is to publicize this change so we can start converting this over as a group.

Comment 2

[Corey Frang [:gnarf]]

Another thing we could do is actually stop testing files that AREN'T in the .jshintwhitelist using gjslint, meaning if it is covered by jshint, let jshint cover it and ignore the file in gjslint.  We can switch linters over one file at a time.

Not quite sure how to pull this one off yet, but it sounds cool in my own echo chamber! :)

Comment 3

[Corey Frang [:gnarf]]

(In reply to James Lal [:lightsofapollo] from comment #1)
> The important thing here is to publicize this change so we can start
> converting this over as a group.

:+1:

I will work on writing a little "how to solve jshint problems" guide to link to that includes some information for how to solve the simple problems (most are missing {} on if's, undefined globals, and undefined exported globals).

One thing I'd be interested to have some help figuring out is how we want to approach solving these jshint problems w/r/t the release cycle and cherry-picking.  Should we suggest that the nastier files wait until bigger refactorings touch the file?  Do we encourage filing a second pull request to fix the jshint errors in a file you've touched that has tiny problems (just need to add some comments to the file to fix it?)

We should collect some thoughts on what might be included in this document here: https://etherpad.mozilla.org/gaia-jshint

Comment 4

[Corey Frang [:gnarf]]

I added the code to only gjslint files listed in .jshintwhitelist

Comment 5

[Julien Wajsberg [:julienw]]

Hey,

my personal plan was to whitelist apps so that "make lint" would effectively be "make hint" for those apps.

The whitelist would be a simple variable in the Makefile (as opposed to a file, KISS) but I'm fine with a file too.

Of course this requires more changes in the Makefile, so I'm fine with doing a simpler solution for now.

I think we should not show all the errors for now, because this is scary for apps that haven't been converted yet. Maybe the error count is enough?

Also my pull requests in jshint have not been merged yet ([1]), this is imo necessary before we can move forward... Could be helpful to get in touch with Anton on IRC.

Will have a look probably today.

[1] https://github.com/jshint/jshint/pulls/julienw

Comment 6

[Corey Frang [:gnarf]]

Hey - I almost think per-file is easier than per-app.

I can alter the reporter so that it silences a bunch of errors in a file, i.e. only report the first 5 errors per file unless --verbose (or not using the whitelist reporter)

Comment 7

[Corey Frang [:gnarf]]

Made a few more updates to the draft doc with julien in the etherpad - output in this gist for now: https://gist.github.com/gnarf/fd757e83990d1fefab22

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 830590 [details] [review]
github PR

I've added comments on github.

Specifically, I think we should try to have `make lint` work like before, only running jshint for some files and gjslint for others.

Comment 9

[Anthony Ricaud (:rik)]

Comment on attachment 830590 [details] [review]
github PR

Thanks a lot for working on this! This is a crucial part of moving to jshint.

I've mostly focused my review on the "workflow" that it enables rather than the code correctness (I trust Julien's review for that). Before landing this, please check that it works correctly on Linux and OS X (we've had many issues in the past). Also, a green Travis :)

Comment 10

[Corey Frang [:gnarf]]

I've updated the PR to meet some of the suggestions.

- build/jshint-xfail.js (instead of jshint-whitelist)
- build/jshint-xfail.list (instead of .jshintwhitelist)

make lint - will now run make gjslint and make hint, silences xfail's from jshint
make gjslint - runs files from build/jshint-xfail.list only
make hint - runs all files, showing first 5 errors in an xfail'ed file

----

I encourage everyone to take a look at the start of the documentation for switching at https://gist.github.com/gnarf/fd757e83990d1fefab22 - post comments, and/or tweak with the writing in the etherpad linked from within please.

Another question we need to solve before this can actually be merged - Where do we host the final copy of that document?

Comment 11

[Corey Frang [:gnarf]]

Comment on attachment 830590 [details] [review]
github PR

Poking this up for review again - A lot of the suggestions were incorporated.

I will bring the jshint-xfail.list up to date with latest master whenever we consider landing this.

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 830590 [details] [review]
github PR

added comments on the pull request

The main issues I see are:
* the size of the command line with the new GJSLINTED_PATH. I feel we should run the command in the shell with a pipe instead.
* the commit hook should not need the network to run

and some nits 

Thanks!

Comment 13

[Julien Wajsberg [:julienw]]

Taking it because Corey asked me to.

Comment 14

[Anthony Ricaud (:rik)]

Comment on attachment 830590 [details] [review]
github PR

Just a few nits for myself. I think this is ready to land and we can improve later. This is too cool to wait more iterations.

One warning: I'm seeing two jshint failures in the Travis build for that PR. They need to be fixed before or when landing this.

Comment 15

[Corey Frang [:gnarf]]

Awesome! I'm going to let julien carry this one home.  Looking forward to seeing it in action though!

Comment 16

[Julien Wajsberg [:julienw]]

Attachment: New github PR

waiting for a green travis before asking review

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 8357857 [details] [review]
New github PR

The "linters" line is green in Travis!


FTR the line to generate the xfail list:

  LC_ALL=C ./node_modules/.bin/jshint apps shared | grep -v '^$' | awk -F: '{ print $1 }' | sort | uniq > build/jshint/xfail.list 

And then remove the first line.

All the changes I made to Corey's work is in a separate commit.

Comment 18

[Corey Frang [:gnarf]]

Comment on attachment 8357857 [details] [review]
New github PR

LGTM! Lets land this sucker!

Comment 19

[Mike Pennisi [:jugglinmike]]

Oooooh boy!

Comment 20

[Julien Wajsberg [:julienw]]

BTW, the tests I have done with this code:

* commit non js files
* commit only gjslinted files (but several of them to test the "new line -> space" conversion)
* commit only jshinted files (but several of them)
* commit both of them (several of both types)

And here are the changes I made to Corey's initial work:

Travis:
* one travis job

Makefile:
* do not try to lint non-existent files

Hook:
* use the jshint executable directly in the hook, so that we never run npm
  install when we just want to commit
* always run both gjslint and jshint, even if one of them fails
* use NO_XFAIL=1 because we already warn about xfailed files
* use LC_ALL=C so that sorting is the same for all platforms

Various:
* move the jshint-specific files to directory build/jshint/
* various other small changes
* update the xfail list

Comment 21

[Anthony Ricaud (:rik)]

Comment on attachment 8357857 [details] [review]
New github PR

Overall it's good.

Problems:
- xargs -d doesn't work on OS X.
- If I don't have jshint installed, the pre-commit hook is failing.

Also, the command to generate the fail list is not sorting consistently. Not a big deal cause we just need to generate it once.

Comment 22

[Anthony Ricaud (:rik)]

Some recommendations on how to land this:
- When jshint fails, we should display a link to the short tutorial on how to fix jshint errors.
- At least 24 hours before landing this, send a message to dev.gaia about the upcoming change. It needs to explain that all new files will be linted via jshint. Also with a link to the tutorial.

Comment 23

[Julien Wajsberg [:julienw]]

The new command to generate the fail list is:

( export LC_ALL=C ; ./node_modules/.bin/jshint apps shared | awk -F: '{ print $1 }' | sort | uniq | grep '\.js$' ) > build/jshint/xfail.list

Comment 24

[Julien Wajsberg [:julienw]]

Updated the etherpad in https://etherpad.mozilla.org/gaia-jshint

Should this be exported to a jshint.md file somewhere ? In build/jshint ? In the root directory ? What do you think ?

Comment 25

[Julien Wajsberg [:julienw]]

Pushed a new version to the PR with 2 additional commits:
* one fixing the comments from Rik, and some little more changes
* one to fix the new jshint issues occuring because of the previous "little more changes". Please tell me if this should go to another bug.

I still need to test more so I won't request review today as I need to move to another bug.

Comment 26

[Corey Frang [:gnarf]]

I suppose if you put it as README.md in `build/jshint/README.md` then a link to `https://github.com/mozilla-b2g/gaia/tree/master/build/jshint` would be enough to show the readme - makes it pretty easily edited, and also near the "target material"

Probably the best place for it!

Comment 27

[Anthony Ricaud (:rik)]

I like that idea.

Comment 28

[Greg Weng [:snowmantw][:gweng][:λ]]

Can we add some useful rules in the .jshintrc of the while Gaia? I'm not to flame a syntax war, but at least we should use

{
  esnext: true
}

to pass ES6 features. As I know, one problem we encountered with gjslinter is some ES6 features would not pass the check.

The all features are listed at: http://www.jshint.com/docs/options/

(Other features, like `laxcomma`, may be too arguable so I would avoid to discuss them here...)

Comment 29

[Julien Wajsberg [:julienw]]

Greg, esnext is already set, as is laxcomma.

See https://github.com/mozilla-b2g/gaia/blob/master/.jshintrc for the current configuration file.

Comment 30

[Greg Weng [:snowmantw][:gweng][:λ]]

Ohh, I just checked the PR. Eager to see the day we completed the migration!

Comment 31

[Julien Wajsberg [:julienw]]

Comment on attachment 8357857 [details] [review]
New github PR

Pushed follow-ups to the PR. All the changed code is in a separate commit for easier review.

Here are the changes:

jshint configuration:
* remove all predefined globals from the main .jshintrc default global
* add new jshintrc files for subdirectories

Hook:
* fix the commit hook to fail when jshint is absent
* bailout early if we don't have xfail.list
* add a link to the README file if some xfailed files are commited

Makefile:
* add documentation at the top of the Makefile
* fix Makefile for Mac OS X and other BSD system: don't use `xargs -d`
* support VERBOSE=1 when running make hint
* add a link to the README file when "make hint" is failing

General:
* fix the xfail list sorting and generate a current list
* add the README file

Comment 32

[Julien Wajsberg [:julienw]]

Mail sent to the dev-gaia mailing list: https://groups.google.com/forum/#!topic/mozilla.dev.gaia/I-ldZN9jrGo

Comment 33

[Julien Wajsberg [:julienw]]

Just pushed a small change to fix the fact that "npm install" was being run because of the backticks in a double quote string.

Comment 34

[Anthony Ricaud (:rik)]

Comment on attachment 8357857 [details] [review]
New github PR

r+ with one nit. Please also link to the README file when pre-commit fails with jshint_result > 0

Comment 35

[Julien Wajsberg [:julienw]]

Pull request changed according to the comment, ready to land.

Comment 36

[Julien Wajsberg [:julienw]]

master: a2ae854a26063c3714420172cc763721a2de78b7

Thanks to all !