Related: we have an open bug to do this (plus https://github.com/prettier/prettier to format the code a la gofmt)  as a presubmit check for infra repo. https://bugs.chromium.org/p/chromium/issues/detail?id=868502

This seems a great feature to work on. I would learn a lot of new things making this. But I'll also ask more questions I guess. :P
Can I work on it?

Hi! You're of course welcome to work on this, although I would be slow answering questions since I'm still on leave.

As noted in the initial post, this should be similar to some other analyzers, but I don't know how easy it will be to fetch the dependencies, which I think will be Node.js and eslint. I think we can assume that we're going to run it only on one platform (like x64 Linux) so if there's a prebuilt binary for Node.js somewhere that might work.

Hi, I made it work locally, but I am confused about cipd.yaml file. Locally, I made a symlink from /usr/bin/node to node folder and /usr/local/lib/node_modules/eslint to eslint folder for the function to work. How do I make these dependencies install in cipd?
The WIP code is at https://chromium-review.googlesource.com/c/infra/infra/+/1293189 for reference.

As a super-quick response: CIPD (Chrome Infra Package Deployment) is a package deployment system; packages are basically zip files. I think that CIPD stores packages using Google Cloud Storage, or something like that.

As long as the CIPD client can package all of the required things before uploading it should be OK. But in order for others to also be able to upload new versions, there should be some automated way of fetching a particular version of the dependencies; it shouldn't rely on the version on your local machine.

Also added a quick comment on that CL.

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/89189bba189f4143b5fc38e4bbfe2c56b9bd2c10

commit 89189bba189f4143b5fc38e4bbfe2c56b9bd2c10
Author: Parminder Singh <parmsingh129@gmail.com>
Date: Wed Nov 21 17:35:04 2018

Eslint Analyser on tricium

Implemented an eslint analyser based on pylint analyser.
Implemented the unit tests and other configs needed.
Implemented a python script to setup node and eslint.

Bug:  874483 
Change-Id: Ida0eb6586d05a89f5f00b012b91a205f033fae14
Reviewed-on: https://chromium-review.googlesource.com/c/1293189
Auto-Submit: Parminder Singh <parmsingh129@gmail.com>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#19121}
[add] https://crrev.com/89189bba189f4143b5fc38e4bbfe2c56b9bd2c10/go/src/infra/tricium/functions/eslint/setup.py
[add] https://crrev.com/89189bba189f4143b5fc38e4bbfe2c56b9bd2c10/go/src/infra/tricium/functions/eslint/test/tricium/data/files.json
[add] https://crrev.com/89189bba189f4143b5fc38e4bbfe2c56b9bd2c10/go/src/infra/tricium/functions/eslint/eslint_parser.go
[add] https://crrev.com/89189bba189f4143b5fc38e4bbfe2c56b9bd2c10/go/src/infra/tricium/functions/eslint/README.md
[add] https://crrev.com/89189bba189f4143b5fc38e4bbfe2c56b9bd2c10/go/src/infra/tricium/functions/eslint/eslint_parser_test.go
[add] https://crrev.com/89189bba189f4143b5fc38e4bbfe2c56b9bd2c10/go/src/infra/tricium/functions/eslint/Makefile
[add] https://crrev.com/89189bba189f4143b5fc38e4bbfe2c56b9bd2c10/go/src/infra/tricium/functions/eslint/test/example.js
[add] https://crrev.com/89189bba189f4143b5fc38e4bbfe2c56b9bd2c10/go/src/infra/tricium/functions/eslint/.eslintrc
[add] https://crrev.com/89189bba189f4143b5fc38e4bbfe2c56b9bd2c10/go/src/infra/tricium/functions/eslint/.gitignore
[add] https://crrev.com/89189bba189f4143b5fc38e4bbfe2c56b9bd2c10/go/src/infra/tricium/functions/eslint/cipd.yaml
[add] https://crrev.com/89189bba189f4143b5fc38e4bbfe2c56b9bd2c10/go/src/infra/tricium/functions/eslint/eslint.infra_testing

Alright! next step is for me to upload a CIPD package for the new analyzer, and enable it on a test repo to try it out :-)

Assigning this to myself now since the issue tracker won't let me assign to parmsingh129@gmail.com (not in the list of project members), although as we can see the change above was written by Parminder.

The following revision refers to this bug:
  https://chromium.googlesource.com/playground/gerrit-tricium/+/24572232da36dbb569b8a51dbff2c5d6a93159d5

commit 24572232da36dbb569b8a51dbff2c5d6a93159d5
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Wed Nov 21 23:02:34 2018

Enable ESLint in tricium playground repo

Bug:  874483 
Change-Id: I03e154df17d0ccc2a40355dbd5391f26eba2195a
[modify] https://crrev.com/24572232da36dbb569b8a51dbff2c5d6a93159d5/tricium-dev.cfg

Example test CL with comments:
https://chromium-review.googlesource.com/c/playground/gerrit-tricium/+/1351436/8/example.js

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/infradata/config/+/851e87dd1b4b268971cfa93ee1ba6c78f4acc5be

commit 851e87dd1b4b268971cfa93ee1ba6c78f4acc5be
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Thu Nov 29 23:40:00 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/357d4b11c550a22a19c317a7ae163172f8a4ff0c

commit 357d4b11c550a22a19c317a7ae163172f8a4ff0c
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Thu Nov 29 23:50:26 2018

[tricium eslint] Change default rc file to match current infra projects

The first place we'll want to enable eslint is in the infra repo;
some things in https://chromium.googlesource.com/infra/infra.git
that use eslint now include Monorail and chopsui:

 - https://cs.chromium.org/chromium/infra/appengine/monorail/.eslintrc.json
 - https://cs.chromium.org/chromium/infra/crdx/chopsui-npm/.eslintrc.json

This set of rules is basically consistent with what we use in general
for Chromium, i.e. basically the same as the Google style guide:

 - https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md#JavaScript
 - https://google.github.io/styleguide/jsguide.html

Bug:  874483 
Change-Id: Iec4ce9c7717063499db542cc33d4714aa95c9867
Reviewed-on: https://chromium-review.googlesource.com/c/1355543
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#19267}
[modify] https://crrev.com/357d4b11c550a22a19c317a7ae163172f8a4ff0c/go/src/infra/tricium/functions/eslint/setup.py
[add] https://crrev.com/357d4b11c550a22a19c317a7ae163172f8a4ff0c/go/src/infra/tricium/functions/eslint/package-lock.json
[modify] https://crrev.com/357d4b11c550a22a19c317a7ae163172f8a4ff0c/go/src/infra/tricium/functions/eslint/eslint_parser.go
[add] https://crrev.com/357d4b11c550a22a19c317a7ae163172f8a4ff0c/go/src/infra/tricium/functions/eslint/.eslintrc.json
[modify] https://crrev.com/357d4b11c550a22a19c317a7ae163172f8a4ff0c/go/src/infra/tricium/functions/eslint/Makefile
[delete] https://crrev.com/41c7710837aac2f93180cde0d77ca8347d4a1012/go/src/infra/tricium/functions/eslint/.eslintrc
[modify] https://crrev.com/357d4b11c550a22a19c317a7ae163172f8a4ff0c/go/src/infra/tricium/functions/eslint/.gitignore
[modify] https://crrev.com/357d4b11c550a22a19c317a7ae163172f8a4ff0c/go/src/infra/tricium/functions/eslint/test/example.js

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/88e0991bef3fb18461c8d62c69cb22fb01b4c8de

commit 88e0991bef3fb18461c8d62c69cb22fb01b4c8de
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Fri Nov 30 19:40:39 2018

Enable ESLint analyzer in the infra repository

This would make it so that all changes to .js files
(but not .html files) would get eslint warnings.

The default eslintrc follows the google config
with 2-space indent and would warn on deviations
from that.

Bug:  874483 
Change-Id: I61d8fb887eb91145ce6214a772c1329088c06d37
Reviewed-on: https://chromium-review.googlesource.com/c/1357203
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#19279}
[modify] https://crrev.com/88e0991bef3fb18461c8d62c69cb22fb01b4c8de/infra/config/global/tricium-dev.cfg

We got comments from ESLint!

https://chromium-review.googlesource.com/c/infra/infra/+/1356554/6/eslinttest/mr-edit-issue.js

However, the list of warnings probably has to be tweaked, and eslint fails if there are more than a few warnings, see  bug 910746 .

Sorry to ask this after so much work has gone into it already, but why should ESLint be applied as a tricium analyzer instead of say, a presubmit check?

We'd really prefer to catch ES lint problems before they get into code review.  Doing it as comments in code review seems like it's:
- later in the process than it needs to be
- more asynchronous than it needs to be
- easy for authors to disregard


Of course we could just write our own presubmit checks in front of this, and use the same config files as the tricium analyzer.  But it seems like duplicated effort.

It's not an either/or -- we should also use linters to catch things in pre-upload hooks, where possible. In general, I believe we should run a small number of very fast, 100% certain checks before upload, and a larger number of checks post-upload.

That is, even if a particular linter is included in a pre-upload hook, we can use the same linter with more checks post-upload, including some checks that have some false positives.

Checks in code review have a few other interesting properties compared to a pre-upload hooks (not necessarily all always positive, but still worth noting):
 - They apply for the whole project consistently.
 - They cannot be quietly skipped by some developers.
 - They can be points of discussion for reviewers.
 - They don't require developers to have linter tools installed locally.
 - There's no concern of pre-upload latency when adding more tools to run.

This analzyer has now been added and the initial version is running for the infra repo.