Closed Bug 1177933 Opened 4 years ago Closed 4 years ago

Implement `mach eslint`

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: mcomella, Assigned: nalexander)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Attachment #8626827 - Flags: review?(nalexander)
mcomella: neat-o.  Me and dmose are interested in this.
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/...|.
(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.
Bug 1177933 - Add |mach eslint| command. r?mcomella,dmose
Attachment #8627383 - Flags: review?(michael.l.comella)
Attachment #8627383 - Flags: review?(dmose)
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.
(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...
(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 on attachment 8626827 [details]
MozReview Request: Bug 1177933 - Add eslint run script. r=nalexander

I reviewed in the form of a counter commit :)
Attachment #8626827 - Flags: review?(nalexander)
(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 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.
Attachment #8627383 - Flags: review?(michael.l.comella) → review+
Assignee: michael.l.comella → nalexander
Attachment #8626827 - Attachment is obsolete: true
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.
Attachment #8626827 - Flags: review+
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 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.
Attachment #8627383 - Flags: review?(dmose) → review+
Thanks for making this happen, by the way!
Summary: Provide run script or mach command for eslint in mobile/android → Implement `mach eslint`
url:        https://hg.mozilla.org/integration/fx-team/rev/0105f77365edd07b16f13a3286ffc92b49701218
changeset:  0105f77365edd07b16f13a3286ffc92b49701218
user:       Nick Alexander <nalexander@mozilla.com>
date:       Thu Jul 02 12:18:52 2015 -0700
description:
Bug 1177933 - Add |mach eslint| command. r=mcomella,dmose
DONTBUILD NPOTB
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.
Status: NEW → ASSIGNED
https://reviewboard.mozilla.org/r/12221/#review10879

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

Not really, but it's habit.
https://hg.mozilla.org/mozilla-central/rev/0105f77365ed
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
(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!
You need to log in before you can comment on or make changes to this bug.