1177933 - Implement `mach eslint`

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Currently we require users to run `eslint . --ext "[.js,.jsm]"` from mobile/android and it's sucky. Let's write a shell script or a mach command.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1177933 - Add eslint run script. r=nalexander

Bug 1177933 - Add eslint run script. r=nalexander

Using mach had complications based on where configuration files were defined so
I made something specific to the mobile team.

Comment 2

[Nick Alexander :nalexander [he/him]]

mcomella: neat-o.  Me and dmose are interested in this.

Comment 3

[Nick Alexander :nalexander [he/him]]

mcomella: I should review this in the next day or two.  I intend to review in the form of a counter-patch, making this a mach command usable more widely.  Something like |mach eslint mobile/android| or |mach eslint toolkit/...|.

Comment 4

[Mark Banner (:standard8)]

(In reply to Nick Alexander :nalexander from comment #3)
> mcomella: I should review this in the next day or two.  I intend to review
> in the form of a counter-patch, making this a mach command usable more
> widely.  Something like |mach eslint mobile/android| or |mach eslint
> toolkit/...|.

You might want to xref bug 1067071, but it'd be great to push forward with some form of consistent command here regardless.

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1177933 - Add |mach eslint| command. r?mcomella,dmose

Bug 1177933 - Add |mach eslint| command. r?mcomella,dmose

Comment 6

[Nick Alexander :nalexander [he/him]]

mcomella, dmose: a skim of the Python would be nice.  This is good to go if it works for you both locally.

Pity that eslint requires hoop-jumping rather than doing good things with relative paths.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Nick Alexander :nalexander from comment #6)
> mcomella, dmose: a skim of the Python would be nice.  This is good to go if
> it works for you both locally.

I'm pretty sure you'll need a build peer (e.g. gps) to review this patch before it goes in, FWIW.

> Pity that eslint requires hoop-jumping rather than doing good things with
> relative paths.

I know that Standard8 has found the eslint maintainer to be quite responsive to filed bugs that are actually functionality problems.  You might consider raising an issue on eslint for this...

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #7)
> (In reply to Nick Alexander :nalexander from comment #6)
> > mcomella, dmose: a skim of the Python would be nice.  This is good to go if
> > it works for you both locally.
> 
> I'm pretty sure you'll need a build peer (e.g. gps) to review this patch
> before it goes in, FWIW.

I'm the Android build peer; for something like this I'm not too concerned about additional eyes.  But now I'd best flag gps, just in case :)
 
> > Pity that eslint requires hoop-jumping rather than doing good things with
> > relative paths.
> 
> I know that Standard8 has found the eslint maintainer to be quite responsive
> to filed bugs that are actually functionality problems.  You might consider
> raising an issue on eslint for this...

It was raised and closed: see https://github.com/eslint/eslint/issues/1382.  (This is from the code comment.)

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8626827 [details]
MozReview Request: Bug 1177933 - Add eslint run script. r=nalexander

I reviewed in the form of a counter commit :)

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Nick Alexander :nalexander from comment #8)
> > > Pity that eslint requires hoop-jumping rather than doing good things with
> > > relative paths.
> > 
> > I know that Standard8 has found the eslint maintainer to be quite responsive
> > to filed bugs that are actually functionality problems.  You might consider
> > raising an issue on eslint for this...
> 
> It was raised and closed: see https://github.com/eslint/eslint/issues/1382. 
> (This is from the code comment.)

Note: After investigating one of the referenced issues, it seems the work around is to move the ignore file to the root of the project but I'm guessing there are some IDE/editor integration issues that would prevent us from doing this.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8627383 [details]
MozReview Request: Bug 1177933 - Add |mach eslint| command. r?mcomella,dmose

https://reviewboard.mozilla.org/r/12221/#review10879

LGTM - thanks for pulling this over the finish line, Nick!

Since we have to be concerned about cwd, I wonder if we should whitelist valid cwds (e.g. if I run `eslint mobile/android/modules` with an ignore of `mobile/android/.eslintignore`, it'd spit out a lot of warnings that were already specified to be ignored).

::: python/mach_commands.py:139
(Diff revision 1)
> +    def eslint(self, ext=None, path=None, args=[]):

If `@CommandArgument` supplies a default value, is `ext=None` necessary?

::: python/mach_commands.py:110
(Diff revision 1)
> -                ensure_exit_code=False, # Don't throw on non-zero exit code.
> +                ensure_exit_code=False,  # Don't throw on non-zero exit code.

Unsure if this was intended, but you have some unrelated whitespace changes in here. In the future, I'd prefer a separate commit but imo it's not worth the time to fix now.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8626827 [details]
MozReview Request: Bug 1177933 - Add eslint run script. r=nalexander

https://reviewboard.mozilla.org/r/12159/#review10895

It'd be nice to have a switch to specify the eslint binary path, as ours is specified in the tree in packages.json, and our scripts explicitly invoke that version.  That said, I don't think that needs to block this landing.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

https://reviewboard.mozilla.org/r/12159/#review10895

In particular, an environment variable would work well, as it wouldn't need to be specified on the command line each time.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8627383 [details]
MozReview Request: Bug 1177933 - Add |mach eslint| command. r?mcomella,dmose

https://reviewboard.mozilla.org/r/12221/#review10939

Looks like Michael has you covered on the python bits.  It works great from the top-level with a path specified.  If I run it while in browser/components/loop, it complains about not having a patch.  It might be nice to default the path to . if none is given.  That, too, is just a nice-to-have and doesn't need to block this patch from landing.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Thanks for making this happen, by the way!

Comment 16

[Nick Alexander :nalexander [he/him]]

url:        https://hg.mozilla.org/integration/fx-team/rev/0105f77365edd07b16f13a3286ffc92b49701218
changeset:  0105f77365edd07b16f13a3286ffc92b49701218
user:       Nick Alexander <[e-mail]>
date:       Thu Jul 02 12:18:52 2015 -0700
description:
Bug 1177933 - Add |mach eslint| command. r=mcomella,dmose
DONTBUILD NPOTB

Comment 17

[Nick Alexander :nalexander [he/him]]

dmose: thanks for the suggestions.  I included a --binary flag, then try to fish the binary from $ESLINT, and finally search your path.  I also defaulted the path to '.'.

As for fishing binaries from package.json, it looks like ESLINT is actually set in your run-all-loop-tests.sh file.  Feel free to push a follow-up to get your desired behaviour.

Comment 18

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/12221/#review10879

> If `@CommandArgument` supplies a default value, is `ext=None` necessary?

Not really, but it's habit.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0105f77365ed

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Nick Alexander :nalexander from comment #17)
> dmose: thanks for the suggestions.  I included a --binary flag, then try to
> fish the binary from $ESLINT, and finally search your path.  I also
> defaulted the path to '.'.
> 
> As for fishing binaries from package.json, it looks like ESLINT is actually
> set in your run-all-loop-tests.sh file.  Feel free to push a follow-up to
> get your desired behaviour.

Great; thanks!