1180345 - Whitelist acceptable `mach eslint` paths

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

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

The issue is that it is recommended to run with the .eslintignore file at the root of the project because it is only searched for in the working directory. Even if a file is used outside the project root (via `--ignore-path`), the paths specified in the ignore file are relative *not* to the ignore file (as would be intuitive and as other programs, such as git, do) but to the working directory. Therefore running `mach eslint mobile/android/base` would not use the proper ignore file (since it's at mobile/android) and the output would be unexpected.

It looks like the current paths are:

/browser/components/loop/
/browser/devtools/
/mobile/android/
/toolkit/devtools/

Based on the position of .eslintignore.

Dan, Nick – is this correct?

Comment 1

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

(In reply to Michael Comella (:mcomella) from comment #0)
> Based on the position of .eslintignore.

https://mxr.mozilla.org/mozilla-central/find?string=.eslintignore&tree=mozilla-central&hint=

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I don't use eslint, so probably not a good person to ask about it.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #0)
> The issue is that it is recommended to run with the .eslintignore file at
> the root of the project because it is only searched for in the working
> directory. Even if a file is used outside the project root (via
> `--ignore-path`), the paths specified in the ignore file are relative *not*
> to the ignore file (as would be intuitive and as other programs, such as
> git, do) but to the working directory. Therefore running `mach eslint
> mobile/android/base` would not use the proper ignore file (since it's at
> mobile/android) and the output would be unexpected.
> 
> It looks like the current paths are:
> 
> /browser/components/loop/
> /browser/devtools/
> /mobile/android/
> /toolkit/devtools/
> 
> Based on the position of .eslintignore.
> 
> Dan, Nick – is this correct?

I don't really know.  It seems possible (even desirable?) that we eventually have no .eslintignore at all.  I really don't care what we do here:

* restrict to paths with .eslintrc (which might handle subpaths badly);
* restrict to paths with .eslintignore (which isn't very future proof and uses .eslintignore as a sentinel);
* check for .eslintignore higher up the tree and modify the path;
* warn in some situations (like the above).

People who actually use eslint should argue from a position of experience, as it becomes wide-spread.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Nick Alexander :nalexander from comment #3)
> (In reply to Michael Comella (:mcomella) from comment #0)
> > The issue is that it is recommended to run with the .eslintignore file at
> > the root of the project because it is only searched for in the working
> > directory. Even if a file is used outside the project root (via
> > `--ignore-path`), the paths specified in the ignore file are relative *not*
> > to the ignore file (as would be intuitive and as other programs, such as
> > git, do) but to the working directory. Therefore running `mach eslint
> > mobile/android/base` would not use the proper ignore file (since it's at
> > mobile/android) and the output would be unexpected.
> > 
> > It looks like the current paths are:
> > 
> > /browser/components/loop/
> > /browser/devtools/
> > /mobile/android/
> > /toolkit/devtools/
> > 
> > Based on the position of .eslintignore.
> > 
> > Dan, Nick – is this correct?
> 
> I don't really know.  It seems possible (even desirable?) that we eventually
> have no .eslintignore at all.  I really don't care what we do here:
> 
> * restrict to paths with .eslintrc (which might handle subpaths badly);

I guess I've been assuming that whitelisting would mean "run only from that directory"; not "run only from that directory or any subdirs".

Implementing the former definition seems like a reasonable plan to me.

> * restrict to paths with .eslintignore (which isn't very future proof and
> uses .eslintignore as a sentinel);

This seems fragile.

> * check for .eslintignore higher up the tree and modify the path;

I _think_, but am not sure, that this would just fail on the loop Gecko stuff.

> * warn in some situations (like the above).

"Run from whitelisted directory, warn and run from subdirs of whitelisted directory" might be ok, though I'd mildly lean towards the strict version.

> People who actually use eslint should argue from a position of experience,
> as it becomes wide-spread.

Adding Standard8 for his thoughts...

Comment 5

[Mark Banner (:standard8)]

I don't really have an opinion here, except it seems to be a defect in eslint, especially as .eslintrc files seem to work logically.

Has this been raised with them?

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Banner (:standard8) from comment #5)
> I don't really have an opinion here, except it seems to be a defect in
> eslint, especially as .eslintrc files seem to work logically.
> 
> Has this been raised with them?

Yes, and it was closed: https://github.com/eslint/eslint/issues/1382

I'd love for somebody more invested to pursue.

Comment 7

[Mark Banner (:standard8)]

Looks like there's similar issues been raised in the past as well:

https://github.com/eslint/eslint/issues/1601

So I'm not sure we'll change their mind, though we might.

However, I've just come across something interesting. I copied browser/components/loop/.eslintignore into the root of the repo, subsequent to that I prepended browser/components/loop/ to all the paths.

Then I ran this from the root of the repo.

eslint --ext .js --ext .jsm --ext .jsx browser/components/loop/

This worked fine, I proved to myself that it picked up the root .eslintignore file, and that the .eslintrc file in browser/components/loop was still respected for those files.

It also looks like my sublime setup picks up the .eslintignore file properly with this method as well (although admittedly that's sometimes a disadvantage for files that only just fail).

Therefore, although I'd still like .eslintignore in individual directories in some way, I think we could just do this with a big global file.

We could probably even land it with a blanket ignoring of most directories, and maybe a generic .elintrc (well, that's probably going too far for this bug).

Comment 8

[Mark Banner (:standard8)]

I believe this is now working fine. We've had a top-level .eslintignore for a while, and no-one has been complaining about where you can/can't run the command.