eslint linter should automatically ignore stuff from tools/rewriting/ThirdPartyPaths.txt

RESOLVED FIXED in Firefox 65

Status

enhancement
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: kats, Assigned: ahal)

Tracking

unspecified
mozilla65

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

Looks like eslint has its own ~/.eslintignore file for folders to skip linting on. It should probably also skip all the things in tools/rewriting/ThirdPartyPaths.txt - currently it does not.
Note that mozlint does read this file globally already:
https://searchfox.org/mozilla-central/source/tools/lint/mach_commands.py

If possible mozlint will try to filter these out automatically, but in some cases it can't so relies on the underlying implementation to do so. In this case these paths can all be accessed via `config['exclude']` here:
https://searchfox.org/mozilla-central/source/tools/lint/eslint/__init__.py#45

It looks like eslint has a --ignore-pattern argument which we should be able to forward those into.
This is pretty much a duplicate of bug 1366714 - at least until we decide to move to dedicated lint plugins for editors.

I guess we could do this, but we should determine if we're going to upset developers by moving third party stuff out of the ignore file.
Normally eslint handles its own file exclusions, but there are still some
globally excluded paths that |mach lint| passes in (e.g objdirs and things in
ThirdPartyPaths.txt).

This makes sure that if they show up in the 'config', we handle them.
The above fixes it.

Though a downside is that it'll make the eslint command longer which might make it easier for Windows users to hit the max command length.
Feel free to push back on this if you had concerns.
(In reply to Mark Banner (:standard8) from comment #2)
> I guess we could do this, but we should determine if we're going to upset
> developers by moving third party stuff out of the ignore file.

If that's actually a sticking point (though I'm not sure why it would be), then there's no harm in using the paths from ThirdPartyPaths.txt as well as leaving them in .eslintignore. But I think it's important we have a single source of truth for finding third party paths, rather than maintain a separate duplicate copy in .eslintignore.
I guess your concern is more centered with the config used by editor integrations, in which case I should clarify: I don't care if we duplicate 3rd party stuff in .eslintignore, as long as we *also* apply the stuff in ThirdPartyPaths.txt so there's no chance of things getting out of sync.
Ok, I think this is fine for now, though we need to figure out the longer term future of how we manage all of this.
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c8e417bfccfb
[eslint] Ignore excluded files that mozlint wasn't able to handle automatically r=Standard8
Sorry, forgot to run a Windows build in my try run (hopefully I'll be able to land bug 1436037 soon and we'll be able to finally pull these tasks out of the build jobs).

Anyway, looks like this failed because we can't find node on the build workers which isn't terribly surprising. Seeing as we'll be porting these tasks to a new workertype soon, I don't want to spend effort solving this. So I'm just going to skip this new test on Windows for now and leave enabling it on Windows as a follow-up.
Flags: needinfo?(ahal)
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a823c2262dbf
[eslint] Ignore excluded files that mozlint wasn't able to handle automatically r=Standard8
https://hg.mozilla.org/mozilla-central/rev/a823c2262dbf
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Target Milestone: Firefox 65 → mozilla65
You need to log in before you can comment on or make changes to this bug.