Closed Bug 1203520 Opened 9 years ago Closed 9 years ago

We need a DevTools ESLint plugin containing our ruleset

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: miker, Assigned: miker)

References

Details

Attachments

(13 obsolete files)

Rather than requiring people to add a rules dir we have decided to create an npm package called eslint-plugin-mozilla-devtools. This plugin will contain the following rules:

1. components-imports: Adds the filename of imported files e.g. Cu.import("some/path/Blah.jsm") adds Blah to the global scope.

2. onlylazygetters: Detects places where lazy getters should have been used but weren't.

3. import-headjs-globals: Import globals from head.js and from any files that were imported by head.js (as far as we can correctly resolve the path).

4. mark-test-function-used: Simply marks test (the test method) as used. This avoids ESHint telling us that the function is never called.

The default value for all four rules is 1 (warn).

The GitHub repo for this plugin will be:
https://github.com/mozilla/eslint-plugin-mozilla-devtools

Issues will be at:
https://github.com/mozilla/eslint-plugin-mozilla-devtools/issues
Attached patch eslint-plugin-1203520.patch (obsolete) — Splinter Review
The bulk of the patch is here:
https://github.com/mozilla/eslint-plugin-mozilla-devtools

We can use r+ and r- in this bug as a general indicator of the review status.

This patch:
1. Adds the mozilla-devtools plugin to .eslintrc
2. Removes a bunch of globals from .eslintrc
3. Corrects a couple of outdated esLint settings e.g. no-empty-class is now no-empty-character-class.
4. Switches no-undef to 1... we can use 2 when our source is in better condition.
5. Removes a heckuvalotta /* eslint globals blah, blah */ comments.
6. Fixes inspector.js so that Esprima is able to parse it.

Installation:
$ git clone git@github.com:mozilla/eslint-plugin-mozilla-devtools.git
$ cd eslint-plugin-mozilla-devtools
$ npm link -g # I think you need -g if eslint is installed globally

I don't think you will need to restart sublime but you might.

Uninstallation:
$ cd eslint-plugin-mozilla-devtools
$ npm unlink -g eslint-plugin-mozilla-devtools

If you could give feedback on the plugin source in GitHub that would be awesome.

When we are happy with it I will publish it with npm.
Attachment #8659255 - Flags: review?(pbrosset)
What do you think about making the plugin accessible for other part of the firefox code base (i.e. naming it something like eslint-plugin-mozilla)?  It looks like most of these rules would be helpful for other places in the code base, and I believe each project could opt in only for things they wanted using their eslint config files (to avoid using something like onlylazygetters that may not be relevant everywhere).

If we shared a single plugin file for firefox then that would simplify installation instructions for people setting it up since they wouldn't have to track down the proper npm package for the portion of the code base they are looking at.  And maybe installation could somehow be automated via `./mach eslint`?

OTOH, there wouldn't be a clear owner for the rules so changing one could cause linting failure in other parts of the code base.  I'd imagine the custom rules that only matter for a particular project could be prefixed with "devtools-", "loop-", etc.
Do es lint plugins have to be hosted on npm?  There'd be an advantage of landing this in-tree, which is if and when we have build failures in automation due to linting errors, we would need to start tagging the plugin revision used in the tree to prevent updates to the npm package from breaking older versions of the code.
(In reply to Brian Grinstead [:bgrins] from comment #5)
> What do you think about making the plugin accessible for other part of the
> firefox code base (i.e. naming it something like eslint-plugin-mozilla)?  It
> looks like most of these rules would be helpful for other places in the code
> base, and I believe each project could opt in only for things they wanted
> using their eslint config files (to avoid using something like
> onlylazygetters that may not be relevant everywhere).
> 
> If we shared a single plugin file for firefox then that would simplify
> installation instructions for people setting it up since they wouldn't have
> to track down the proper npm package for the portion of the code base they
> are looking at.  And maybe installation could somehow be automated via
> `./mach eslint`?
> 
> OTOH, there wouldn't be a clear owner for the rules so changing one could
> cause linting failure in other parts of the code base.  I'd imagine the
> custom rules that only matter for a particular project could be prefixed
> with "devtools-", "loop-", etc.

Yeah, the name can be anything and I would prefer something fairly generic. Other teams are very interested in the components-imports rules.

At the moment all rules are warnings by default but they can be off by default and each team could have the appropriate rules activated in their .eslintrc... I think that is the way to go but at the moment it is simplified to make it easier to review.

Of course, if you have any more ideas for rules this would be the place to mention it.
(In reply to Brian Grinstead [:bgrins] from comment #6)
> Do es lint plugins have to be hosted on npm?  There'd be an advantage of
> landing this in-tree, which is if and when we have build failures in
> automation due to linting errors, we would need to start tagging the plugin
> revision used in the tree to prevent updates to the npm package from
> breaking older versions of the code.

You make a good point.

I still think it should be a node package as that simplifies configuration in your editor but it could certainly live in tree. mach could be used to link it locally... that would be a symlink from /usr/local/lib/node_modules/ to the plugin in the tree.
Comment on attachment 8659255 [details] [diff] [review]
eslint-plugin-1203520.patch

Review of attachment 8659255 [details] [diff] [review]:
-----------------------------------------------------------------

The code changes in this patch look good to me.
Why are the new rules you created in the eslint plugin not being used in .eslintrc in this patch?
They seem to be configured by default in the plugin itself, but I'm wondering if this is the right approach. Maybe they should all be off by default, so that each user of this plugin can configure which ones they want to enable in .eslintrc instead.

I agree we need to change the name of the github repo and the name of the eslint plugin to be more mozilla-generic. Can you change this?

I also agree that, in time, we should try and have the plugin in the tree so we can synchronize with the code better, but the approach with the github repo and npm seems good to me for now. It's a good way to get started quickly.
We'll need to make sure the plugin is published on npm before this patch lands though.
We'll also need to make sure the documentation [1] is updated and that an email is sent on the mailing list. Indeed, people are going to have to install the plugin using npm, or ESlint will stop working as soon as we land the patch (it will crash trying to find the uninstalled plugin).

[1] https://wiki.mozilla.org/DevTools/CodingStandards#JS_linting_with_ESLint

I'm going to do the rest of the review on github now.

::: browser/devtools/.eslintrc
@@ +244,5 @@
>      "no-trailing-spaces": 2,
>      // Disallow use of undeclared variables unless mentioned in a /*global */
>      // block.
>      // This should really be a 2, but until we define all globals in comments
>      // and .eslintrc, keeping this as a 1.

This comment should be updated to say that we have an eslint rule for this.

::: browser/devtools/.eslintrc.mochitests
@@ +7,5 @@
>      // Allow using undefined variables so that tests can refer to functions
>      // and variables defined in head.js files, without having to maintain a
>      // list of globals in each .eslintrc file.
>      // Note that bug 1168340 will eventually help auto-registering globals
>      // from head.js files.

This comment should be updated to say that now, undefined variables cause warnings, but that our special eslint rule should auto-register things correctly.
Attachment #8659255 - Flags: review?(pbrosset)
For info, I'm doing the code review of the eslint plugin on github by means of a pull request. I've forked the repo and will submit a PR soon with code changes and comments marked as [CODE REVIEW]. Github's diff tool should nicely highlight all my comments.
Attached patch eslint-plugin-1203520.patch (obsolete) — Splinter Review
@patrick: I have addressed your comments so can you go to https://github.com/mozilla/eslint-plugin-mozilla/commits/master and review my commits from Sep 11 onwards... I am a lot happier with the state of this code.

I still need to create a mach install but I will do that as part of the hg patch.
Attachment #8659255 - Attachment is obsolete: true
Attachment #8661138 - Flags: review?(pbrosset)
Attached patch eslint-plugin-1203520.patch (obsolete) — Splinter Review
I have now moved the plugin into the tree under /testing/eslint-plugin-mozilla/. I discussed this with the testing guys a while back and they are fine with it as long as I am a peer over that folder.

I will remove the git repository to avoid any confusion.

Everything is now in the attached patch. Installation etc is as follows:
1. ./mach eslint-setup
2. We check for node and if it isn't installed:
"Could not find nodejs! Please install nodejs from https://nodejs.org and try again."
3. Offer to install eslint
4. Offer to install eslint-plugin-mozilla
5. Offer to install eslint-plugin-react

Yes installs and No uninstalls inline with mach mercurial-setup.
Attachment #8661138 - Attachment is obsolete: true
Attachment #8661138 - Flags: review?(pbrosset)
Attachment #8661818 - Flags: review?(pbrosset)
Comment on attachment 8661818 [details] [diff] [review]
eslint-plugin-1203520.patch

Review of attachment 8661818 [details] [diff] [review]:
-----------------------------------------------------------------

The code changes make sense. And moving the whole thing in tree is nice. Thanks for working on the eslint-setup command at the same time too.

About the command though: I've been talking about who should review the mach_commands.py changes in #developers but no one seemed to know for sure. The thing doesn't look like it's really owned. I suggest getting a review from :gps anyway, maybe he can forward to someone else.

During the discussion, a good point came out (from :glandium): we have too many mach commands already and we do already have mach eslint. Why not expand on this existing command instead. Make it prompt to install eslint first and configure the right rules, etc...?
This command already says to use npm to install eslint and the react plugin.
I know you changed that to invite the user to run eslint-setup first, but we could make the eslint command do this instead of adding yet another one.

By the way this command lives in a different mach_commands.py file: https://dxr.mozilla.org/mozilla-central/source/python/mach_commands.py#135 (this was reviewed by :mcomella and :dmose, maybe you could flag them for discussion/review instead).

The other thing is I tried running the command locally (on windows 10) and hit 2 road blocks:
- first one is, if you're using the windows mozilla-build tools (like most people installing the dev environment for the first time by reading doc on mdn are), then node can't be found. Even if you install it on your machine, this shell environment doesn't see it. You have to add it to the path first. I wonder if there's anything we can do to make this simpler.
- once done, I tried running the command and answered Yes to "would you like to use eslint" and got this:

WindowsError: [Error 2] The system cannot find the file specified

  File "c:\Users\Patrick\dev\fx-team\tools/mach_commands.py", line 484, in eslint_setup
    stdout = fnull)
  File "c:\mozilla-build\python\lib\subprocess.py", line 522, in call
    return Popen(*popenargs, **kwargs).wait()
  File "c:\mozilla-build\python\lib\subprocess.py", line 710, in __init__
    errread, errwrite)
  File "c:\mozilla-build\python\lib\subprocess.py", line 958, in _execute_child
    startupinfo)

::: testing/eslint-plugin-mozilla/docs/rules/import-headjs-globals.md
@@ +14,5 @@
> +
> +If path does not exist because it is generated e.g. `testdir + "/somefile.js"`
> +we do our best to resolve it.
> +
> +We also include `browser/devtools/framework/test/shared-head.js`

The eslint-plugin-mozilla plugin isn't only for devtools, but instead for any project in tree that wants to use it. Also shared-head.js is included with Services.scriptloader.loadSubScript, so shared-head.js should be included automatically right? So no need to mention this here.

::: testing/eslint-plugin-mozilla/docs/rules/only-lazy-getters.md
@@ +4,5 @@
> +
> +Detects places where lazy getters could have been used but weren't.
> +
> +The following are examples of rule output:
> +

So I have 2 problems with this rule as is:
- 'loader' here refers to the devtools loader, right (resource://gre/modules/devtools/Loader.jsm)? But this rule seems to be pretty generic, which might confuse people into thinking they can use it outside of devtools. Why not name this rule like only-devtools-lazy-getters ? And make sure the documentation says that this is for when your using the devtools loader.
- About the "Please consider making `require` lazy" bit, my problem with it is that it's impossible for the rule to check whether whatever is being required is actually used all the time or not. So really, using require or lazyRequireGetter is a developer's choice, and having ESLint warn us about this all the time, no matter what, may be frustrating.

::: testing/eslint-plugin-mozilla/lib/helpers.js
@@ +119,5 @@
> +   *        The variable name to add to the current scope.
> +   * @param {ASTContext} context
> +   *        The current context.
> +   */
> +  addVarToScope: function(name, context) {

Is this the hack (using eslint internals) you were mentioning in https://github.com/eslint/eslint/issues/3743 ?
If so, then it'd be nice adding a comment somewhere here mentioning this and the issue.

::: testing/eslint-plugin-mozilla/lib/rules/components-imports.js
@@ +22,5 @@
> +
> +  return {
> +    ExpressionStatement: function(node) {
> +      var source = helpers.getSource(node, context);
> +      var name = helpers.getVarNameFromImportSource(source);

Adding that helpers.js file really did wonders on this rule. Really short and sweet.

::: testing/eslint-plugin-mozilla/lib/rules/import-headjs-globals.js
@@ +69,5 @@
> +
> +          var fileName = path.basename(filePath);
> +          if (fileName === "shared-head.js") {
> +            // shared-head.js is a special case and we know the path so let's
> +            // access it directly.

Do we need this special devtools-only case here? What about if another shared-head.js gets added somewhere else in the tree?
I think we need to make sure this is treated generically.

::: testing/eslint-plugin-mozilla/package.json
@@ +9,5 @@
> +    "mozilla",
> +    "firefox"
> +  ],
> +  "bugs": {
> +    "url": "https://github.com/mozilla/eslint-plugin-mozilla/issues"

This repo doesn't exist anymore. Could use the bugzilla URL instead.

::: tools/mach_commands.py
@@ +4,3 @@
>  
>  from __future__ import absolute_import, unicode_literals
>  

This file probably requires a build peer review. Can you flag :gps please?
Attachment #8661818 - Flags: review?(pbrosset) → review?(gps)
Comment on attachment 8661818 [details] [diff] [review]
eslint-plugin-1203520.patch

I'll ask gps for review when I have addressed your Patrick's comments.
Attachment #8661818 - Flags: review?(gps)
Attached patch eslint-plugin-1203520.patch (obsolete) — Splinter Review
Now works on Windows.
Attachment #8662883 - Attachment is obsolete: true
Attached patch eslint-plugin-1203520.patch (obsolete) — Splinter Review
Removed trailing spaces... works just fine on Windows 10.

@gps: As I said above, you only really need to look at python/mach_commands.py.
Attachment #8662985 - Attachment is obsolete: true
Attachment #8662993 - Flags: review?(gps)
Comment on attachment 8662993 [details] [diff] [review]
eslint-plugin-1203520.patch

Review of attachment 8662993 [details] [diff] [review]:
-----------------------------------------------------------------

You'll also want to update .gitignore.

I only reviewed the Python bits, as I'm not an appropriate person to look at this JS.

Given the volume of review comments, I'd like to see another version before granting review.

Also, you are using Mercurial: please use MozReview so I can look at interdiffs when reviewing.

::: python/mach_commands.py
@@ +35,5 @@
>  '''.strip()
>  
> +NODE_NOT_FOUND_MESSAGE = '''
> +nodejs is either not installed or installed to a non-standard path.
> +Please install nodejs from https://nodejs.org and try again.

Message should say something about PATH

@@ +41,5 @@
> +
> +NPM_NOT_FOUND_MESSAGE = '''
> +Node Package Manager (npm) is either not installed or installed to a
> +non-standard path. Please install npm from https://nodejs.org (it comes as an
> +option in the node installation) and try again.

Ditto.

@@ +216,5 @@
>              cwd=path,
>              pass_thru=True,  # Allow user to run eslint interactively.
>              ensure_exit_code=False,  # Don't throw on non-zero exit code.
>          )
> +    def eslint_setup(self, update_only=False):

This should ideally be part of `mach bootstrap`. But I'll let it live here since eslint is still a niche use case used by a small minority of developers (although we should expand it to all JS at some point).

@@ +227,5 @@
> +        sys.path.append(os.path.dirname(__file__))
> +
> +        # At the very least we need node installed.
> +        nodePath = self.getNodePath()
> +        if nodePath == "":

if not nodePath:

@@ +232,5 @@
> +            print(NODE_NOT_FOUND_MESSAGE)
> +            return 1
> +
> +        npmPath = self.getNPMPath()
> +        if npmPath == "":

if not npmPath:

@@ +249,5 @@
> +            return 1
> +
> +        # Offer to install eslint-plugin-mozilla.
> +        print("")
> +        if self._prompt_yn(ESLINT_PLUGIN_MOZILLA_PROMPT):

It seems strange that we would not want to install these plugins.

Does npm/node not allow you to install plugins to additional locations? i.e. does it not have a search path like Python and almost every other dynamic language? If it does, we should install Mozilla-specific Node modules into ~/.mozbuild (self.mozbuild_state_path IIRC) and adjust the node invocation to search for modules in this location. Even better would be if we could run these plugins direct from the source tree. Otherwise people have to run the setup to ensure the linter plugins are up to date. That will lead to all kinds of issues with environments being out of sync.

@@ +283,5 @@
> +        if platform.system() == "Windows":
> +            nodePaths = [
> +                "C:/nodejs/node.exe",
> +                "C:/Program Files/nodejs/node.exe",
> +                "C:/Program Files (x86)/nodejs/node.exe"

You really need a good reason to hard code paths like this because there are so many ways it can break.

@@ +290,5 @@
> +            for nodePath in nodePaths:
> +                if self.is_valid(nodePath):
> +                    return nodePath
> +
> +            return ""

return None

@@ +293,5 @@
> +
> +            return ""
> +        else:
> +            try:
> +                nodePath = which.which('node')

We should be trying which.which first because looking at PATH is the proper way to find executables.

@@ +297,5 @@
> +                nodePath = which.which('node')
> +                return nodePath
> +            except which.WhichError:
> +                print(NODE_NOT_FOUND_MESSAGE)
> +                return ""

return None

@@ +299,5 @@
> +            except which.WhichError:
> +                print(NODE_NOT_FOUND_MESSAGE)
> +                return ""
> +
> +    def getNPMPath(self):

Same comments as above. Also, there is room to consolidate this code into a shared function.

@@ +328,5 @@
> +        try:
> +            with open(os.devnull, "w") as fnull:
> +                subprocess.check_call([path, "--version"], stdout=fnull)
> +                return True
> +        except:

bareword except is dangerous because it traps *all* exceptions, not just those deriving from Exception. The exception that gets thrown after ctrl+c is notably not derived from Exception. This should be "except Exception" or even better "except subprocess.CalledProcessError"
Attachment #8662993 - Flags: review?(gps)
Attached patch eslint-plugin-1203520.patch (obsolete) — Splinter Review
Addressed review comments... still needs testing on Windows 10.
Attachment #8662993 - Attachment is obsolete: true
Comment on attachment 8664256 [details] [diff] [review]
eslint-plugin-1203520.patch

Review of attachment 8664256 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this in a way which isn't devtools-specific!

::: devtools/.eslintrc
@@ +2,5 @@
>    "env": {
>      "es6": true
>    },
> +  "plugins": [
> +    "mozilla"

It would be nice if this was in a top-level .eslintrc
(In reply to Matthew N. [:MattN] (behind on mail) from comment #24)
> Comment on attachment 8664256 [details] [diff] [review]
> eslint-plugin-1203520.patch
> 
> Review of attachment 8664256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for doing this in a way which isn't devtools-specific!
> 
> ::: devtools/.eslintrc
> @@ +2,5 @@
> >    "env": {
> >      "es6": true
> >    },
> > +  "plugins": [
> > +    "mozilla"
> 
> It would be nice if this was in a top-level .eslintrc
If you're only talking about that bit (the mozilla plugin and the es6 env), I agree, we could create a global eslintrc file that the devtool inherits from.
If, however, you're talking about the whole eslintrc file (with the rules), I think that'd be good, but might be hard: agreeing on a set of code-style rules is kind of hard. It's essential that a team shares a set of conventions to write/format code, but it also means everyone has to be on board. That's why we started small and created an eslintrc config file just for devtools to start with.
It'd be great if those conventions went all the way up to the global level and applied to all JS code in mozilla, but that might trigger a lot of discussions.
Yeah, I meant the plugin part to start since I don't think it has any effect on rules without subdirectories enabling the plugin.
Bug 1203520 - We need a DevTools ESLint plugin containing our ruleset
Attachment #8666623 - Flags: review?(gps)
Attachment #8664256 - Attachment is obsolete: true
@gps: Please only review the python stuff... the js files are all reviewed.
Note to self: Move .eslint plugins mozilla line to the root folder.
Moved plugin config to the .eslintrc in the root folder r?=pbrosset
Attachment #8666644 - Flags: review?(pbrosset)
Comment on attachment 8666644 [details]
MozReview Request: Moved plugin config to the .eslintrc in the root folder r?=pbrosset

https://reviewboard.mozilla.org/r/20589/#review18421
Attachment #8666644 - Flags: review?(pbrosset) → review+
Attachment #8666623 - Flags: review?(gps)
Comment on attachment 8666623 [details]
MozReview Request: Bug 1203520 - We need a DevTools ESLint plugin containing our ruleset

https://reviewboard.mozilla.org/r/20587/#review18469

::: python/mach_commands.py:26
(Diff revision 1)
> +from sets import Set

`Set` is in the global scope as a built-in type. The `sets` module is deprecated and should not be used. This line is unnecessary.

::: python/mach_commands.py:235
(Diff revision 1)
> +        nodePath = self.getNodeOrNpmPath("node.exe")

The .exe munging should be done in getNodeOrNpmPath.

::: python/mach_commands.py:239
(Diff revision 1)
> +        npmPath = self.getNodeOrNpmPath("npm.cmd")

Ditto.

::: python/mach_commands.py:247
(Diff revision 1)
> +                            stdout = fnull, stderr = fnull)

Nit: No spaces around "=" in named arguments. Ditto for below as well.

::: python/mach_commands.py:271
(Diff revision 1)
> +        return Set([

return {
   X,
   Y,
}

{ } is set literal syntax. { } is also dict literal syntax. But dicts have {k: value} form whereas sets only have comas around symbols.

::: python/mach_commands.py:272
(Diff revision 1)
> +            os.path.join(os.environ.get("SystemDrive"), os.sep, "nodejs"),

You problably want `"%s\\nodejs" % os.environ.get("SystemDrive")` since calling os.path.join with "\" as an argument is a bit weird. I can't even remember what it does. No-ops?

::: python/mach_commands.py:308
(Diff revision 1)
> +        elif platform.system() == "Darwin":
> +            print("  - /usr/local/bin/node")
> +        elif platform.system() == "Linux":
> +            print("  - /usr/bin/nodejs")

What's the point of these lines?

::: testing/eslint-plugin-mozilla/docs/rules/components-imports.md:1
(Diff revision 1)
> +# components-imports

We have in-tree ReStructuredText docs. If you rewrote this as rst, you could hook it up so the docs magically appear at https://gecko.readthedocs.org/en/latest/. And the docs for doing that hook-up are on the bottom of that page :)
https://reviewboard.mozilla.org/r/20587/#review18469

> What's the point of these lines?

It gives the valid node and npm installation paths for each OS so for osx we would have:
"nodejs is either not installed or is installed to a non-standard path. Please install nodejs from https://nodejs.org and try again.

Valid installation paths:
  - /usr/local/bin/node

> We have in-tree ReStructuredText docs. If you rewrote this as rst, you could hook it up so the docs magically appear at https://gecko.readthedocs.org/en/latest/. And the docs for doing that hook-up are on the bottom of that page :)

Nobody seems to know about this but I really like it. I have converted our documentation.
Comment on attachment 8666623 [details]
MozReview Request: Bug 1203520 - We need a DevTools ESLint plugin containing our ruleset

Bug 1203520 - We need a DevTools ESLint plugin containing our ruleset
Comment on attachment 8666644 [details]
MozReview Request: Moved plugin config to the .eslintrc in the root folder r?=pbrosset

Moved plugin config to the .eslintrc in the root folder r?=pbrosset
Addressed review comments and converted documentation to .rst format r?=gps
Attachment #8667329 - Flags: review?(gps)
Comment on attachment 8667329 [details]
MozReview Request: Addressed review comments and converted documentation to .rst format r?=gps

https://reviewboard.mozilla.org/r/20709/#review18593

::: python/mach_commands.py:287
(Diff revision 1)
>              appPaths = self.getPossibleNodePathsWin()
>  
>              for appPath in appPaths:
> -                nodeOrNpmPath = os.path.join(appPath, filename)
> +                nodeOrNpmPath = os.path.join(appPath, filename + ".exe")
> +                if self.is_valid(nodeOrNpmPath):
> +                    return nodeOrNpmPath
> +
> +                nodeOrNpmPath = os.path.join(appPath, filename + ".cmd")
>                  if self.is_valid(nodeOrNpmPath):
>                      return nodeOrNpmPath

I just realized that which.which() takes an option "path" argument that is an iterable of paths to search. If not defined, it defaults to the PATH env variable.

So, you can replace this with `which.which(filename, path=self.getPossibleNodePathsWin())`
Attachment #8667329 - Flags: review?(gps) → review+
Attachment #8666623 - Flags: review?(gps) → review+
Comment on attachment 8666623 [details]
MozReview Request: Bug 1203520 - We need a DevTools ESLint plugin containing our ruleset

https://reviewboard.mozilla.org/r/20587/#review18595
Comment on attachment 8666623 [details]
MozReview Request: Bug 1203520 - We need a DevTools ESLint plugin containing our ruleset

Bug 1203520 - We need a DevTools ESLint plugin containing our ruleset
Comment on attachment 8666644 [details]
MozReview Request: Moved plugin config to the .eslintrc in the root folder r?=pbrosset

Moved plugin config to the .eslintrc in the root folder r?=pbrosset
Comment on attachment 8667329 [details]
MozReview Request: Addressed review comments and converted documentation to .rst format r?=gps

Addressed review comments and converted documentation to .rst format r?=gps
Restore .eslintrc changes lost by Access Violation on rebase r+=me
Attachment #8667828 - Flags: review?(mratcliffe)
Comment on attachment 8667828 [details]
MozReview Request: Restore .eslintrc changes lost by Access Violation on rebase r+=me

Restore .eslintrc changes lost by Access Violation on rebase r+=me
Comment on attachment 8667828 [details]
MozReview Request: Restore .eslintrc changes lost by Access Violation on rebase r+=me

Restore .eslintrc changes lost by Access Violation on rebase r+=me
Comment on attachment 8667828 [details]
MozReview Request: Restore .eslintrc changes lost by Access Violation on rebase r+=me

Restore .eslintrc changes lost by Access Violation on rebase r+=me
Attachment #8667828 - Flags: review?(mratcliffe) → review+
Comment on attachment 8667828 [details]
MozReview Request: Restore .eslintrc changes lost by Access Violation on rebase r+=me

https://reviewboard.mozilla.org/r/20839/#review18705

$ hg rebase -b 1203520-eslint-plugin.patch -d fx-team
  rebasing 264955:556bfe1c3d10 "Bug 1203520 - We need a DevTools ESLint plugin containing our ruleset"
  merging devtools/.eslintrc
  exception at 980D723C:
  Access violation.
  merging devtools/server/actors/inspector.js
  merging devtools/shared/output-parser.js
  rebasing 264956:42f7eb5b03c5 "Moved plugin config to the .eslintrc in the root folder r?=pbrosset"
  merging devtools/.eslintrc
  rebasing 264957:f3f2c3ffeebc "Addressed review comments and converted documentation to .rst format r?=gps" (1203520-eslint-plugin.patch)

Because of the Access violation I had lost my changes to .eslintrc, hence this commit.
Comment on attachment 8666623 [details]
MozReview Request: Bug 1203520 - We need a DevTools ESLint plugin containing our ruleset

Bug 1203520 - We need a DevTools ESLint plugin containing our ruleset
Comment on attachment 8666644 [details]
MozReview Request: Moved plugin config to the .eslintrc in the root folder r?=pbrosset

Moved plugin config to the .eslintrc in the root folder r?=pbrosset
Comment on attachment 8667329 [details]
MozReview Request: Addressed review comments and converted documentation to .rst format r?=gps

Addressed review comments and converted documentation to .rst format r?=gps
Comment on attachment 8667828 [details]
MozReview Request: Restore .eslintrc changes lost by Access Violation on rebase r+=me

Restore .eslintrc changes lost by Access Violation on rebase r+=me
Use getPossibleNodePathsWin() as the path param in which.which() r+=miker
Attachment #8667881 - Flags: review?(mratcliffe)
Comment on attachment 8667881 [details]
MozReview Request: Use getPossibleNodePathsWin() as the path param in which.which() r+=miker

https://reviewboard.mozilla.org/r/20849/#review18713
Attachment #8667881 - Flags: review?(mratcliffe) → review+
(In reply to Pulsebot from comment #54)
> https://hg.mozilla.org/integration/fx-team/rev/1264e01362f3

Of course... I forgot about your standalone version of Loop.
https://hg.mozilla.org/mozilla-central/rev/4fd815c46a67
https://hg.mozilla.org/mozilla-central/rev/1264e01362f3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Blocks: 1211477
Blocks: 1211478
Attachment #8666623 - Attachment is obsolete: true
Attachment #8666644 - Attachment is obsolete: true
Attachment #8667329 - Attachment is obsolete: true
Attachment #8667881 - Attachment is obsolete: true
Attachment #8667828 - Attachment is obsolete: true
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.