pinterest / arcanist-linters

A collection of custom Arcanist linters
Apache License 2.0
63 stars 45 forks source link

eslint arc formatter #43

Open longlho opened 4 years ago

longlho commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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

longlho commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

^ 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 commented 3 years ago

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

yarn add -D eslint-formatter-arcanist

This warned:

warning " > eslint-formatter-arcanist@1.0.0" 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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