Closed
Bug 1177933
Opened 9 years ago
Closed 9 years ago
Implement `mach eslint`
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox42 fixed)
RESOLVED
FIXED
Firefox 42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: mcomella, Assigned: nalexander)
References
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.
Reporter | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
mcomella: neat-o. Me and dmose are interested in this.
Assignee | ||
Comment 3•9 years ago
|
||
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•9 years ago
|
||
(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.
Assignee | ||
Comment 5•9 years ago
|
||
Bug 1177933 - Add |mach eslint| command. r?mcomella,dmose
Attachment #8627383 -
Flags: review?(michael.l.comella)
Attachment #8627383 -
Flags: review?(dmose)
Assignee | ||
Comment 6•9 years ago
|
||
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•9 years ago
|
||
(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...
Assignee | ||
Comment 8•9 years ago
|
||
(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.)
Assignee | ||
Comment 9•9 years ago
|
||
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)
Reporter | ||
Comment 10•9 years ago
|
||
(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.
Reporter | ||
Comment 11•9 years ago
|
||
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+
Reporter | ||
Updated•9 years ago
|
Assignee: michael.l.comella → nalexander
Reporter | ||
Updated•9 years ago
|
Attachment #8626827 -
Attachment is obsolete: true
Comment 12•9 years ago
|
||
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+
Comment 13•9 years ago
|
||
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•9 years ago
|
||
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+
Comment 15•9 years ago
|
||
Thanks for making this happen, by the way!
Reporter | ||
Updated•9 years ago
|
Summary: Provide run script or mach command for eslint in mobile/android → Implement `mach eslint`
Assignee | ||
Comment 16•9 years ago
|
||
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
Assignee | ||
Comment 17•9 years ago
|
||
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
Assignee | ||
Comment 18•9 years ago
|
||
https://reviewboard.mozilla.org/r/12221/#review10879
> If `@CommandArgument` supplies a default value, is `ext=None` necessary?
Not really, but it's habit.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Comment 20•9 years ago
|
||
(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!
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•