Changes to the linter - new "make hint"

[Julien Wajsberg]

Hi,

I just landed bug 885713 [1]. The initial goal is to propose a `make
hint` command that would use jshint instead of gjslint.

I configured it intentionally stricter than what we have with gjslint
currently. My goal here is not to switch tools for the sake of switching
tools, but enforce some more quality. Therefore, please take this as a
tool for your work. The long-term goal is to have better enforced
quality on our code.

The current error count is very big, but a lot of those errors come from
global variables being used, and that is easily fixed by adding a
/*global <objects list> */ at the start of your files. see [2] for an
example. In some files this can lead to a big list, and that's probably
a sign that your app could have a better architecture.

If you want to relax a rule in one of your files, it's also possible of
course.

This is only a new tool, make a good use of it :)

The only current problem I saw is that, contrary to gjslint, it doesn't
relax the "maxlen" rule when we can't do otherwise. I filed [3] for this.

Some additional informations:
* use "APP=<app>" to limit the output to only your app. Works with both
"make lint" and "make hint"
* both share the same ignore file in .jshintignore
* there is a new pre-commit hook that's automatically installed when you
run make profile. Please report if you run into problems as that's
always possible.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=885713
[2] https://github.com/julienw/gaia/compare/sms-jshint-fixes#L0L0
[3] https://github.com/jshint/jshint/issues/1282

Have fun coding !

--
Julien

[Jan Jongboom]

Thanks Julien, I was using JSHint already for my own work and it's great to see a project wide adoption of it.

[Sergi Mansilla]

Nice, thanks!

On Thursday, September 26, 2013 3:31:57 PM UTC+2, Julien Wajsberg wrote:

[Jan Jongboom]

Although it's extremely strict now... Omitting curly braces and enforcing 'use strict' everywhere should be turned of imho.

[Rick Waldron]

On Thu, Sep 26, 2013 at 11:32 AM, Jan Jongboom <janjo...@gmail.com>wrote:

> Although it's extremely strict now... Omitting curly braces and enforcing
> 'use strict' everywhere should be turned of imho.
>

For consistency sake, these are exactly the sort of things to protect
against ;)

Rick

[Gareth Aye]

Thanks Julien! I like jshint :)

On Thu, Sep 26, 2013 at 2:09 PM, Rick Waldron <waldro...@gmail.com>wrote:

> On Thu, Sep 26, 2013 at 11:32 AM, Jan Jongboom <janjo...@gmail.com
> >wrote:
>

> > Although it's extremely strict now... Omitting curly braces and enforcing
> > 'use strict' everywhere should be turned of imho.
> >
>

> For consistency sake, these are exactly the sort of things to protect
> against ;)
>
> Rick

> _______________________________________________
> dev-gaia mailing list
> dev-...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-gaia
>



--
Best,
Gareth

[Julien Wajsberg]

Le 26/09/2013 17:32, Jan Jongboom a écrit :
> Although it's extremely strict now... Omitting curly braces and enforcing 'use strict' everywhere should be turned of imho.
>

you can turn off by file if you want ;)

I personally think these are important. We may try to implement per app
config files later, but for these rules I think they're good.

[Andrew Sutherland]

What is the suggested plan for dealing with files where there are a lot
of things that make jshint angry as it relates to automated uplifts?
Specifically, I am expecting that we will see a lot of requests to
uplift fixes to v1.2 for the next month or so. Especially with changes
to the bracing rules, it seems likely that patches will no longer
automatically uplift if we fix all of those jshint errors.

Should we tell jshint to skip files like that for a month or so until we
think most of the v1.2 uplifts are past? Or is there a helper script
that the automated uplift machinery could use to fix-up diffs for these
changes?

Thanks,
Andrew

[Andrew Sutherland]

On 09/26/2013 09:31 AM, Julien Wajsberg wrote:
> I configured it intentionally stricter than what we have with gjslint
> currently. My goal here is not to switch tools for the sake of switching
> tools, but enforce some more quality. Therefore, please take this as a
> tool for your work. The long-term goal is to have better enforced
> quality on our code.

> * there is a new pre-commit hook that's automatically installed when you
> run make profile. Please report if you run into problems as that's
> always possible.

Adding/changing an enforcement mechanism by default like the pre-commit
seems like something that should have been proposed/discussed on the
list prior to landing, especially in terms of picking the settings in
use. For comparison, changes to the Mozilla code style guide are
discussed on dev-platform prior to making changes to the (non-enforced)
wiki.

Not that I'm saying these aren't good changes. But I would expect a
change like this to provide an opportunity for feedback from the
affected parties before landing, or a decree by the module owners of the
Gaia module. I don't see comments from Vivien or Tim on the bug at all,
although they may have been consulted off bug.

Andrew

[Andrew Sutherland]

On 09/26/2013 03:13 PM, Andrew Sutherland wrote:
> On 09/26/2013 09:31 AM, Julien Wajsberg wrote:
>> I configured it intentionally stricter than what we have with gjslint
>> currently. My goal here is not to switch tools for the sake of switching
>> tools, but enforce some more quality. Therefore, please take this as a
>> tool for your work. The long-term goal is to have better enforced
>> quality on our code.
>> * there is a new pre-commit hook that's automatically installed when you
>> run make profile. Please report if you run into problems as that's
>> always possible.
>
> Adding/changing an enforcement mechanism by default like the
> pre-commit seems like something that should have been
> proposed/discussed on the list prior to landing, especially in terms
> of picking the settings in use. For comparison, changes to the
> Mozilla code style guide are discussed on dev-platform prior to making
> changes to the (non-enforced) wiki.

It looks like I parsed the inclusion of the pre-commit comment in this
context incorrectly and my scan of the diff too quickly. Reading more
thoroughly, it looks like the already-existing pre-commit hook was
cleaned up (excludes in unified .jshintignore file, "make lint" used
instead of direct invocation of gjslint), *but we are still only using
gjslint as part of the pre-commit hook and jshint is a purely optional
thing*.

Which is good, so now we can just debate the right settings for jshint ;)

What's the plan for migrating to using jshint as the pre-commit lint
hook and the travis lint pass?

Andrew

["Yuan Xulei(袁徐磊)"]

Thank you. A cool tool!

When I tried |make hint| under the root directory of gaia,
I got the following error. I use Ubuntu 12.10 64bit and
npm 1.1.4

-----------------------------------------------
npm install --registry http://registry.npmjs.org
npm http GET http://registry.npmjs.org/marionette-client/0.15.5
npm http 304 http://registry.npmjs.org/marionette-client/0.15.5
npm http GET http://registry.npmjs.org/debug
npm http GET http://registry.npmjs.org/json-wire-protocol
npm http GET http://registry.npmjs.org/socket-retry-connect
npm http GET http://registry.npmjs.org/sockit-to-me
npm http 304 http://registry.npmjs.org/debug
npm http 304 http://registry.npmjs.org/json-wire-protocol
npm http GET http://registry.npmjs.org/debug/-/debug-0.6.0.tgz
npm http GET
http://registry.npmjs.org/json-wire-protocol/-/json-wire-protocol-0.2.1.tgz
npm http 304 http://registry.npmjs.org/socket-retry-connect
npm http GET
http://registry.npmjs.org/socket-retry-connect/-/socket-retry-connect-0.0.1.tgz
npm http 304 http://registry.npmjs.org/sockit-to-me
npm ERR! error installing marionet...@0.15.5

npm ERR! Error: No compatible version found: sockit-to-me@'>=0.1.0- <0.2.0-'
npm ERR! No valid targets found.
npm ERR! Perhaps not compatible with your version of node?
npm ERR! at installTargetsError (/usr/share/npm/lib/cache.js:488:10)
npm ERR! at next_ (/usr/share/npm/lib/cache.js:438:17)
npm ERR! at next (/usr/share/npm/lib/cache.js:415:44)
npm ERR! at /usr/share/npm/lib/cache.js:408:5
npm ERR! at saved
(/usr/share/npm/lib/utils/npm-registry-client/get.js:147:7)
npm ERR! at Object.oncomplete (/usr/lib/nodejs/graceful-fs.js:230:7)
npm ERR! You may report this log at:
npm ERR! <http://bugs.debian.org/npm>
npm ERR! or use
npm ERR! reportbug --attach /home/yuan/gaia/npm-debug.log npm
npm ERR!
npm ERR! System Linux 3.5.0-40-generic
npm ERR! command "/usr/bin/nodejs" "/usr/bin/npm" "install" "--registry"
"http://registry.npmjs.org"
npm ERR! cwd /home/yuan/gaia
npm ERR! node -v v0.6.19
npm ERR! npm -v 1.1.4
npm ERR! message No compatible version found: sockit-to-me@'>=0.1.0-
<0.2.0-'
npm ERR! message No valid targets found.
npm ERR! message Perhaps not compatible with your version of node?
npm ERR!
npm ERR! Additional logging details can be found in:
npm ERR! /home/yuan/gaia/npm-debug.log

On 09/26/2013 09:31 PM, Julien Wajsberg wrote:
> Hi,
>
> I just landed bug 885713 [1]. The initial goal is to propose a `make
> hint` command that would use jshint instead of gjslint.
>

> I configured it intentionally stricter than what we have with gjslint
> currently. My goal here is not to switch tools for the sake of switching
> tools, but enforce some more quality. Therefore, please take this as a
> tool for your work. The long-term goal is to have better enforced
> quality on our code.
>

> The current error count is very big, but a lot of those errors come from
> global variables being used, and that is easily fixed by adding a
> /*global <objects list> */ at the start of your files. see [2] for an
> example. In some files this can lead to a big list, and that's probably
> a sign that your app could have a better architecture.
>
> If you want to relax a rule in one of your files, it's also possible of
> course.
>
> This is only a new tool, make a good use of it :)
>
> The only current problem I saw is that, contrary to gjslint, it doesn't
> relax the "maxlen" rule when we can't do otherwise. I filed [3] for this.
>
> Some additional informations:
> * use "APP=<app>" to limit the output to only your app. Works with both
> "make lint" and "make hint"
> * both share the same ignore file in .jshintignore

> * there is a new pre-commit hook that's automatically installed when you
> run make profile. Please report if you run into problems as that's
> always possible.
>

> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=885713
> [2] https://github.com/julienw/gaia/compare/sms-jshint-fixes#L0L0
> [3] https://github.com/jshint/jshint/issues/1282
>
> Have fun coding !
>
>
>

[Gabriele Svelto]

I've noticed a minor inconsistency between 'make lint' and 'make hint',
if I write something like this:

var foo = (function bar() {}).bind(this);

hint will complain:

Wrapping non-IIFE function literals in parens is unnecessary

But if I do this instead:

var foo = function bar() {}.bind(this);

It's lint's turn to complain:

Missing semicolon after function assigned to a variable

Gabriele

[Julien Wajsberg]

Right, I'll see if we can relax this specific rule. I don't want that
the jshint rules oblige you to break our (current) default linter.

Could you please file a bug ?
--
Julien

[Julien Wajsberg]

Le 26/09/2013 21:32, Andrew Sutherland a écrit :
> On 09/26/2013 03:13 PM, Andrew Sutherland wrote:
>> On 09/26/2013 09:31 AM, Julien Wajsberg wrote:

>>> I configured it intentionally stricter than what we have with gjslint
>>> currently. My goal here is not to switch tools for the sake of
>>> switching
>>> tools, but enforce some more quality. Therefore, please take this as a
>>> tool for your work. The long-term goal is to have better enforced
>>> quality on our code.

>>> * there is a new pre-commit hook that's automatically installed when
>>> you
>>> run make profile. Please report if you run into problems as that's
>>> always possible.
>>

>> Adding/changing an enforcement mechanism by default like the
>> pre-commit seems like something that should have been
>> proposed/discussed on the list prior to landing, especially in terms
>> of picking the settings in use. For comparison, changes to the
>> Mozilla code style guide are discussed on dev-platform prior to
>> making changes to the (non-enforced) wiki.
>
> It looks like I parsed the inclusion of the pre-commit comment in this
> context incorrectly and my scan of the diff too quickly. Reading more
> thoroughly, it looks like the already-existing pre-commit hook was
> cleaned up (excludes in unified .jshintignore file, "make lint" used
> instead of direct invocation of gjslint), *but we are still only using
> gjslint as part of the pre-commit hook and jshint is a purely optional
> thing*.

Yep, I should have been clearer in my mail!

>
> Which is good, so now we can just debate the right settings for jshint ;)
>
> What's the plan for migrating to using jshint as the pre-commit lint
> hook and the travis lint pass?

I'd say: file bugs for each app ;)

I don't expect a switch to jshint (if we ever switch) before 6 months.
I already thought of an "opt-in" switch for apps, either using a dotfile
or simply in a var in the Makefile. Could this be a good idea ?

--
Julien

[Gabriele Svelto]

On 27/09/2013 10:51, Julien Wajsberg wrote:
> Right, I'll see if we can relax this specific rule. I don't want that
> the jshint rules oblige you to break our (current) default linter.
>
> Could you please file a bug ?

Here you go :)

https://bugzilla.mozilla.org/show_bug.cgi?id=921398

Gabriele

[Rick Waldron]

On Fri, Sep 27, 2013 at 3:03 AM, Gabriele Svelto <gsv...@mozilla.com>wrote:

> I've noticed a minor inconsistency between 'make lint' and 'make hint', if
> I write something like this:
>
> var foo = (function bar() {}).bind(this);
>
> hint will complain:
>
> Wrapping non-IIFE function literals in parens is unnecessary
>
> But if I do this instead:
>
> var foo = function bar() {}.bind(this);
>
> It's lint's turn to complain:
>
> Missing semicolon after function assigned to a variable

Unfortunately, gjslint has always been very wrong.

Rick

[James Burke]

On Thu, Sep 26, 2013 at 12:13 PM, Andrew Sutherland
<asuth...@asutherland.org> wrote:
> Adding/changing an enforcement mechanism by default like the pre-commit
> seems like something that should have been proposed/discussed on the list
> prior to landing, especially in terms of picking the settings in use. For
> comparison, changes to the Mozilla code style guide are discussed on
> dev-platform prior to making changes to the (non-enforced) wiki.

I really appreciate having the jshint in the tree, as I have been
using it anyway in my editor to check on file saves, and it feels like
a better toolchain option. Thanks for adding it!

However, the .jshintrc rules as currently set do seem to contradict
what I understood to be Mozilla style (maybe I have this wrong
though?). Specifically:

* curly: true: this one mandates braces around single line if/else
bodies. My impression given existing code was that one line, unbraced
bodies were acceptable, even expected, style.
* use strict is true.

For the non-Mozilla code I write, I prefer those settings,
particularly curly: true, but I noticed while doing code changes now
my editor is using the .jshintrc and now I have a bunch of lint errors
because the existing code does not match those settings.

So I would like to see turning those two options off, discussing if we
want a change, then if agreed to change, have a bulk code operation to
update the code, then set the options back on.

If this seems reasonable, I am happy to file the bug to turn off the
settings and do the pull request, while the discussion happens about
any bulk style change.

As it is now, my existing edit/save flow is compromised now that I
have to wade through the new errors. I also do not want to pollute any
pull requests for changes for these rules as it creates more noise for
the reviewer.

James

[Rick Waldron]

On Fri, Sep 27, 2013 at 1:46 PM, James Burke <jrb...@gmail.com> wrote:

> On Thu, Sep 26, 2013 at 12:13 PM, Andrew Sutherland
> <asuth...@asutherland.org> wrote:
> > Adding/changing an enforcement mechanism by default like the pre-commit
> > seems like something that should have been proposed/discussed on the list
> > prior to landing, especially in terms of picking the settings in use.
> For
> > comparison, changes to the Mozilla code style guide are discussed on
> > dev-platform prior to making changes to the (non-enforced) wiki.
>
> I really appreciate having the jshint in the tree, as I have been
> using it anyway in my editor to check on file saves, and it feels like
> a better toolchain option. Thanks for adding it!
>
> However, the .jshintrc rules as currently set do seem to contradict
> what I understood to be Mozilla style (maybe I have this wrong
> though?). Specifically:
>
> * curly: true: this one mandates braces around single line if/else
> bodies. My impression given existing code was that one line, unbraced
> bodies were acceptable, even expected, style.
>

After working my way through the files here:
https://mxr.mozilla.org/mozilla-central/source/dom/phonenumberutils/Subjectively,
I absolutely think this should be changed to mandatory
curlies, all the time, no exceptions. Curly omission is asking for code
confusion.

Rick

[Fabrice Desré]

On 09/27/2013 12:36 PM, Rick Waldron wrote:

>
> After working my way through the files here:
> https://mxr.mozilla.org/mozilla-central/source/dom/phonenumberutils/Subjectively,
> I absolutely think this should be changed to mandatory
> curlies, all the time, no exceptions. Curly omission is asking for code
> confusion.

fwiw, this is also the rule in chrome js code and we enforce it at
review time.

Fabrice
--
Fabrice Desré
b2g team
Mozilla Corporation