Linting before every commit

[Anthony Ricaud]

Hello gaiaers,

In bug 882116, I'm proposing to run the linter in a git pre-commit hook.

If gjslint is not in your path, you can commit but you'll get a message
in your console suggesting to install it.
If gjslint in your path, it won't let you commit if you have lint errors
in the files that you are trying to commit.
The linter will only run on files that have been modified so it should
run very quickly (way faster than |make lint|).

I think that's a reasonable tradeoff but maybe I'm overlooking some
workflows. Anyone has concerns with this idea?

[james]

Sounds awesome.

- james

[Ben Kelly]

On Jun 13, 2013, at 12:44 PM, Anthony Ricaud <ant...@ricaud.me> wrote:
> In bug 882116, I'm proposing to run the linter in a git pre-commit hook.
>
> If gjslint is not in your path, you can commit but you'll get a message in your console suggesting to install it.
> If gjslint in your path, it won't let you commit if you have lint errors in the files that you are trying to commit.
> The linter will only run on files that have been modified so it should run very quickly (way faster than |make lint|).
>
> I think that's a reasonable tradeoff but maybe I'm overlooking some workflows. Anyone has concerns with this idea?

I like the idea of linting, but it made me think of a couple questions:

So prior to modifying a file we should fix all existing linting problems? In a separate commit perhaps? Would this make uplifts harder if we are churning the repo more for lint issues?

What about making lint run as part of the test infrastructure so it shows up in travis?

Thanks!

Ben

[Kevin Grandon]

It's a great idea. Alternatively if you don't do this, you should absolutely wait for travis to pass before landing.

Best,
Kevin

----- Original Message -----
From: "Anthony Ricaud" <ant...@ricaud.me>
To: mozilla-...@lists.mozilla.org
Sent: Thursday, June 13, 2013 9:44:18 AM
Subject: Linting before every commit

Hello gaiaers,

In bug 882116, I'm proposing to run the linter in a git pre-commit hook.

If gjslint is not in your path, you can commit but you'll get a message
in your console suggesting to install it.
If gjslint in your path, it won't let you commit if you have lint errors
in the files that you are trying to commit.
The linter will only run on files that have been modified so it should
run very quickly (way faster than |make lint|).

I think that's a reasonable tradeoff but maybe I'm overlooking some
workflows. Anyone has concerns with this idea?

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

[Gareth Aye]

Waiting for Travis is a great idea! Alternatively, outside of the
Mozilla-sphere a tool called phabricator (a project Facebook
open-sourced) is very popular for bringing together a lot of process
around running linters and tests, doing code reviews, and landing code
in source control. This is how the tool works in broad strokes (after
you set everything up):

git checkout -b your-feature-branch

# do some work
# commit some stuff

arc diff # squashes your commits together
# asks you for things like a reviewer, summary, test
plan, etc.
# runs the lint checks and tests
# sends the patch and lint/test results up to the
phabricator server
# emails the reviewer and anyone that you've cc'd

# reviewer does a review, people comment, there are nice colors
# you have to respond to a few issues

# do some work
# commit some stuff

arc diff # squashes your commits together
# gives you a chance to update patch details
# runs the lint checks and tests
# sends the updated patch and lint/test results up to
the phabricator server
# emails people

# reviewer decides they like your patch and they accept the revision

arc land # squashes everything together
# fetches the latest stuff from the branch you're trying
to land on
# rebases your patch on top
# pushes for you

I think it could be really nice to use for gaia and we could even hack
it to work with Bugzilla. Thoughts?

[John Hu]

This is a great work. I think we should also link the process with unit tests. Currently, travis is often unable to get everything pass on unit test.

BR,
John

[Anthony Ricaud]

On 13/06/13 19:05, Ben Kelly wrote:
> So prior to modifying a file we should fix all existing linting
> problems? In a separate commit perhaps? Would this make uplifts
> harder if we are churning the repo more for lint issues?

Yes, we should fix all lint errors of a file. But 99.9% of the time,
files will have no errors because our linter tests are green. So I don't
think that will be an issue.

> What about making lint run as part of the test infrastructure so it
> shows up in travis?

We already run |make lint| on travis:
https://github.com/mozilla-b2g/gaia/blob/master/tools/ci/unit/travis.sh#L61
https://travis-ci.org/mozilla-b2g/gaia/builds/8086517#L36

(Ironically there is one lint error but I've filed bug 883138 for this)

[Ben Kelly]

On Jun 14, 2013, at 9:49 AM, Anthony Ricaud <ant...@ricaud.me> wrote:
> On 13/06/13 19:05, Ben Kelly wrote:

> > So prior to modifying a file we should fix all existing linting
> > problems? In a separate commit perhaps? Would this make uplifts
> > harder if we are churning the repo more for lint issues?

> Yes, we should fix all lint errors of a file. But 99.9% of the time, files will have no errors because our linter tests are green. So I don't think that will be an issue.
>

> > What about making lint run as part of the test infrastructure so it
> > shows up in travis?

> We already run |make lint| on travis:
> https://github.com/mozilla-b2g/gaia/blob/master/tools/ci/unit/travis.sh#L61
> https://travis-ci.org/mozilla-b2g/gaia/builds/8086517#L36

Yea, sorry. Kevin set me straight on IRC.

Your proposal sounds good to me given the current state.

Thanks!

Ben

[Jan Jongboom]

Great idea!

[Anthony Ricaud]

I just landed the patch.

As an extra benefit, I enabled the multiprocess option of gjslint. That
makes |make lint| n times faster! (From 60 to 15 seconds on my machine)
This was already by default for Linux users.

If you encounter any problem either with the git hook or the
multiprocess option, please open a blocker bug on bug 882116.

[Anthony Ricaud]

On 18/06/13 16:08, Anthony Ricaud wrote:
> I just landed the patch.
>
> As an extra benefit, I enabled the multiprocess option of gjslint. That
> makes |make lint| n times faster! (From 60 to 15 seconds on my machine)
> This was already by default for Linux users.
>
> If you encounter any problem either with the git hook or the
> multiprocess option, please open a blocker bug on bug 882116.

And because it was too beautiful to be true, I had to disable this
because of Travis. (bug 884338)

If anyone wants to improve our Makefile to allow enabling multiprocess
by default with a switch to disable it…

[Gabriele Svelto]

On 18/06/2013 17:08, Anthony Ricaud wrote:
> If anyone wants to improve our Makefile to allow enabling multiprocess
> by default with a switch to disable it…

While building Gaia I noticed that make would fail when using parallel
builds (-jN with N > 1), is this related? I'd be willing to look into
the problem if that's the case, do we already have a bug for it?

Gabriele

[Anthony Ricaud]

No, this is a Travis specific issue.
https://travis-ci.org/mozilla-b2g/gaia/builds/8199015

File "/usr/lib/python2.7/multiprocessing/synchronize.py", line 75, in
__init__
sl = self._semlock = _multiprocessing.SemLock(kind, value, maxvalue)
OSError: [Errno 13] Permission denied

[Kevin Grandon]

Another fix was recently landed in master which will let you actually commit deletes to JS files.

You should probably get the latest master and run: make install-git-hook, otherwise you might see this problem in the future.

Best,
Kevin

----- Original Message -----
From: "Anthony Ricaud" <ant...@ricaud.me>
To: mozilla-...@lists.mozilla.org

Sent: Tuesday, June 18, 2013 8:08:35 AM
Subject: Re: Linting before every commit

On 18/06/13 16:08, Anthony Ricaud wrote:
> I just landed the patch.
>
> As an extra benefit, I enabled the multiprocess option of gjslint. That
> makes |make lint| n times faster! (From 60 to 15 seconds on my machine)
> This was already by default for Linux users.
>
> If you encounter any problem either with the git hook or the
> multiprocess option, please open a blocker bug on bug 882116.
And because it was too beautiful to be true, I had to disable this
because of Travis. (bug 884338)

If anyone wants to improve our Makefile to allow enabling multiprocess
by default with a switch to disable it…

> On 13/06/13 18:44, Anthony Ricaud wrote:
>> Hello gaiaers,
>>
>> In bug 882116, I'm proposing to run the linter in a git pre-commit hook.
>>
>> If gjslint is not in your path, you can commit but you'll get a message
>> in your console suggesting to install it.
>> If gjslint in your path, it won't let you commit if you have lint errors
>> in the files that you are trying to commit.
>> The linter will only run on files that have been modified so it should
>> run very quickly (way faster than |make lint|).
>>
>> I think that's a reasonable tradeoff but maybe I'm overlooking some
>> workflows. Anyone has concerns with this idea?