eslint arc formatter

[longlho]

Hi there,

We (Dropbox) also use ESLint with arc and I happened to stumble upon this repo. We just open-sourced https://github.com/dropbox/eslint-formatter-arcanist as well that deals w/ autofix and arc byte char issue so if you guys are interested feel free to take a look.

[jparise]

Thanks for the heads up, @longlho. Can you share an example of how you're using dropbox/eslint-formatter-arcanist in your Arcanist configuration?

[longlho]

DBX uses bazel so we have a generic bazel-based linter that works with any linter that outputs ArcanistMessage. We then basically just trigger the eslint binary with --format arcanist and that's it. I saw that you guys have something similar here: https://github.com/pinterest/arcanist-linters/blob/master/src/ESLintLinter.php#L111

[jparise]

#79 adds more complete autofix support for our ESLint linter, so I'm going to close this issue.

[longlho]

hmm I saw the PR but looks like it's just using eslint output directly which isn't correct due to encoding issue. See https://github.com/dropbox/eslint-formatter-arcanist/blob/master/tests/index.test.ts test for the test sample.

[RainNapper]

I can work on resolving this issue, but I'm not too clear on how eslint formatters work as implemented in eslint-formatter-arcanist. It seems like it's a matter of whether we translate the output in arc layer or eslint layer. Wanted to see what the benefits of each are, and if we can consolidate our work.

It sounds like eslint-formatter-arcanist is designed for: eslint -> [eslint] middleware to output ArcanistMessage -> [arc] generic linter that forwards ArcanistMessage -> arc

While this is designed for: eslint -> [arc] eslint specific arc linter -> arc

It's not clear to me how to consolidate the two here as the workflows don't match. One idea is to replace logic in ESLintLinter.php with a similar generic linter you described, and update the command use --format arcanist, and require installing eslint-formatter-arcanist in order to use this linter.

Thoughts?

[longlho]

so I think at a high level arc linters are basically just invoking shell cmds and capture stdout. So you can just add --format arcanist to ESLintLinter.php args and capture & map stdout like you already did in the diff (but will slightly different keys ofc).

[RainNapper]

For backwards compatibility, I think we'll need to split the the logic otherwise we will break existing setups that do not have eslint-formatter-arcanist installed. I think we should re-open issue in the meantime and follow-up.

[jparise]

For backwards compatibility, I think we'll need to split the the logic otherwise we will break existing setups that do not have eslint-formatter-arcanist installed. I think we should re-open issue in the meantime and follow-up.

I think this might be something we could handle with a configuration flag (instead of creating an entirely new linter class).

[RainNapper]

^ Agree. I just updated my comment to reflect that. I think we can add a new flag like eslint.format, and branch the logic in parseLinterOutput based on that.

[RainNapper]

@longlho I am working locally to try and use eslint-formatter-arcanist. I ran:

yarn add -D eslint-formatter-arcanist

This warned:

warning " > [email protected]" has incorrect peer dependency "eslint@^7.3.1".

So I updated my eslint to that version, and re-installed.

I am seeing this error:

      '[REPO_DIR]/client/node_modules/.bin/eslint' '--no-color' '--resolve-plugins-relative-to' './client' '--format' 'arcanist' '--config' './client/.eslintrc' '[REPO_DIR]/client/src/ui/flow/flowListItem.js'
      
      STDOUT
      (empty)
      
      STDERR
      There was a problem loading formatter: [REPO_DIR]/node_modules/eslint/lib/cli-engine/formatters/arcanist
      Error: Cannot find module '[REPO_DIR]/node_modules/eslint/lib/cli-engine/formatters/arcanist'
      Require stack:
      - [REPO_DIR]/client/node_modules/eslint/lib/cli-engine/cli-engine.js
      - [REPO_DIR]/client/node_modules/eslint/lib/eslint/eslint.js
      - [REPO_DIR]/client/node_modules/eslint/lib/eslint/index.js
      - [REPO_DIR]/client/node_modules/eslint/lib/cli.js
      - [REPO_DIR]/client/node_modules/eslint/bin/eslint.js

Are the install steps in the repo out of date?

[longlho]

nope it works I just verified it

~/s/random ❯❯❯ npx eslint -f arcanist index.js
[{"char":9,"code":"ESLint","description":"Strings must use singlequote.","line":1,"name":"quotes","original":"\"@formatjs/intl-getcanonicallocales/polyfill\"","path":"/Users/long.ho/src/random/index.js","replacement":"'@formatjs/intl-getcanonicallocales/polyfill'","severity":2},{"char":9,"code":"ESLint","description":"Strings must use singlequote.","line":2,"name":"quotes","original":"\"@formatjs/intl-locale/polyfill\"","path":"/Users/long.ho/src/random/index.js","replacement":"'@formatjs/intl-locale/polyfill'","severity":2},{"char":9,"code":"ESLint","description":"Strings must use singlequote.","line":3,"name":"quotes","original":"\"@formatjs/intl-pluralrules/polyfill\"","path":"/Users/long.ho/src/random/index.js","replacement":"'@formatjs/intl-pluralrules/polyfill'","severity":2},{"char":9,"code":"ESLint","description":"Strings must use singlequote.","line":4,"name":"quotes","original":"\"@formatjs/intl-pluralrules/locale-data/ko\"","path":"/Users/long.ho/src/random/index.js","replacement":"'@formatjs/intl-pluralrules/locale-data/ko'","severity":2},{"char":9,"code":"ESLint","description":"Strings must use singlequote.","line":5,"name":"quotes","original":"\"@formatjs/intl-numberformat/polyfill\"","path":"/Users/long.ho/src/random/index.js","replacement":"'@formatjs/intl-numberformat/polyfill'","severity":2},{"char":0,"code":"ESLint","description":"Expected 1 empty line after require statement not followed by another require.","line":7,"name":"import/newline-after-import","original":"","path":"/Users/long.ho/src/random/index.js","replacement":"\n","severity":2},{"char":9,"code":"ESLint","description":"Strings must use singlequote.","line":6,"name":"quotes","original":"\"@formatjs/intl-numberformat/locale-data/ko\"","path":"/Users/long.ho/src/random/index.js","replacement":"'@formatjs/intl-numberformat/locale-data/ko'","severity":2},{"char":1,"code":"ESLint","description":"Unexpected console statement.","line":7,"name":"no-console","original":null,"path":"/Users/long.ho/src/random/index.js","replacement":null,"severity":1},{"char":1,"code":"ESLint","description":"Expected indentation of 2 spaces but found 4.","line":8,"name":"indent","original":"    ","path":"/Users/long.ho/src/random/index.js","replacement":"  ","severity":2},{"char":1,"code":"ESLint","description":"Expected indentation of 4 spaces but found 8.","line":9,"name":"indent","original":"        ","path":"/Users/long.ho/src/random/index.js","replacement":"    ","severity":2},{"char":1,"code":"ESLint","description":"Expected indentation of 4 spaces but found 8.","line":10,"name":"indent","original":"        ","path":"/Users/long.ho/src/random/index.js","replacement":"    ","severity":2},{"char":1,"code":"ESLint","description":"Expected indentation of 4 spaces but found 8.","line":11,"name":"indent","original":"        ","path":"/Users/long.ho/src/random/index.js","replacement":"    ","severity":2},{"char":0,"code":"ESLint","description":"Missing trailing comma.","line":12,"name":"comma-dangle","original":"","path":"/Users/long.ho/src/random/index.js","replacement":",","severity":2},{"char":1,"code":"ESLint","description":"Expected indentation of 2 spaces but found 4.","line":12,"name":"indent","original":"    ","path":"/Users/long.ho/src/random/index.js","replacement":"  ","severity":2},{"char":0,"code":"ESLint","description":"Missing trailing comma.","line":13,"name":"comma-dangle","original":"","path":"/Users/long.ho/src/random/index.js","replacement":",","severity":2},{"char":0,"code":"ESLint","description":"Missing semicolon.","line":14,"name":"semi","original":"","path":"/Users/long.ho/src/random/index.js","replacement":";","severity":2}]

something's up w/ how your eslint resolve its dep?

[RainNapper]

Issue appears to be with eslint where the cwd can't be overridden, which is impacting its ability to resolve installed packages. I filed https://github.com/eslint/eslint/issues/14731.

[longlho]

yeah so fwiw we ship tools like this with all its dep in a single binary so it's hermetic :)