Page MenuHomePhabricator

Added ESLint Support
Changes PlannedPublic

Authored by michaeljs1990 on Mar 29 2015, 7:01 AM.
Tags
None
Referenced Files
F12838288: D12198.id35057.diff
Thu, Mar 28, 6:03 PM
F12838285: D12198.id39086.diff
Thu, Mar 28, 6:03 PM
F12838273: D12198.id35013.diff
Thu, Mar 28, 6:02 PM
F12838269: D12198.id35016.diff
Thu, Mar 28, 6:02 PM
F12838244: D12198.id35015.diff
Thu, Mar 28, 6:02 PM
F12813131: D12198.id29320.diff
Thu, Mar 28, 12:53 AM
Unknown Object (File)
Sun, Mar 24, 5:08 AM
Unknown Object (File)
Sat, Mar 23, 6:12 PM
Tokens
"The World Burns" token, awarded by yelirekim."Pterodactyl" token, awarded by sascha-egerer."Mountain of Wealth" token, awarded by damio."Pterodactyl" token, awarded by cwang."Baby Tequila" token, awarded by ninacfgarcia."Like" token, awarded by oluckyman."Like" token, awarded by adamchainz."Like" token, awarded by tycho.tatitscheff."Love" token, awarded by turadg."Pterodactyl" token, awarded by chad.

Details

Summary

This allows arcanist to use the JavaScript linter ESLint.

Test Plan

Unit tested

Diff Detail

Repository
rARC Arcanist
Branch
arcpatch-D12198_1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 8823
Build 10294: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I may have messed up my tests somehow they are gone now... not sure how that happened... Let me know if i need to readd them and i'll update.

Can I ask when this will be landed? As I'm really looking forward to this feature!

@davidnormo unfortunately I was working on this for my job and was using it internally before we had to switch off phabricator which makes me very sad. If you would like to take over this diff please do so and see it through :) All the work should be done it just needs some attention. I think I also messed up the unit tests with a commit but if you go back one you just need to copy / paste them in.

@yangsu your https://github.com/coursera/arcanist/blob/master/src/lint/linter/ArcanistESLintLinter.php works well. Would you be willing to commandeer this and merge the functionality?

I'd really like to see this in master so I'm happy to do it too.

@turadg I am going to finish this up tonight and ask for a review again.

Fix unit test issue

Unit test fails with undefined propery of object.
Add in class variables so this doesn't happen.

  • Update After Review
  • Fix bad rebase
  • Arc Liberate
  • Arc Unit Failure

ESLint seems to have changes the way errors/warnings are generated. I'm looking into why the output from the test file seems to have changed now.

Fix Unit tests.

ESLint no longer outputs any errors if you don't have an .eslintrc file set up. I have setup the eslint file with very specific rules since it seems in the past they have made lots of changes and if you use the recommended warnings it may break the linter unit tests for no reason then upstream it outputting a new error or no longer things an error should be output.

Updated so unit tests will now pass and added comments about why the .eslintrc file was added.

src/lint/linter/__tests__/ArcanistESLintLinterTestCase.php
10 ↗(On Diff #35016)

I am guessing that you may use something different then dirname(FILE) for this as you support php 5.2.

src/lint/linter/__tests__/eslint/.eslintrc
1 ↗(On Diff #35016)

This file is needed because ESLint no longer outputs any errors/warnings if you don't have rules set. I have taken care to use rules that will not break in the future or be changed so no one has to spend time debugging upstream changes in ESLint as opposed to actual breaks in the linter.

@yangsu I was looking at the work you had done on this. I was mostly curious about https://github.com/coursera/arcanist/blob/master/src/lint/linter/ArcanistESLintLinter.php#L114-L129. Do you have a js file that reproduces the case you ran into that caused a need for this code?

@joshuaspence you aren't on the subscription list so I'm not sure if you got any notifications about update or if you are still interested in reviewing this PR.

  • I think we now have a means of specifying min version for a linter, and it looks like you could use that feature here.
  • The upstream now requires an open task before considering a diff; You should create one and attach this revision.
src/lint/linter/ArcanistESLintLinter.php
19

This was actually changed since - see T9353.
The idea is that the one-liner output from arc linters should be useful for someone who never used eslint/javascript.

src/lint/linter/ArcanistExternalLinter.php
66 ↗(On Diff #35016)

👍

src/lint/linter/__tests__/ArcanistESLintLinterTestCase.php
10 ↗(On Diff #35016)

what do all other lint test cases do?

I'll open up a task for this right now as well as setting the minimum linter version that can be used. Thanks for the heads up I guess I should have reread the guidelines for submitting a patch as it's been a while since I started this.

src/lint/linter/__tests__/ArcanistESLintLinterTestCase.php
10 ↗(On Diff #35016)

Just checked and every other linter is using this method of getting the directory name so this seems to be the standard.

@avivey I looked around for a way to set min linter version but don't see any formal way to do it. I could write some code to run the check in getDefaultBinary() but this seems messy since none of the others are currently doing it.

  • Update After Review
  • Fix bad rebase
  • Arc Liberate
  • Arc Unit Failure

@avivey I added a "min version" however I think that we need to add a minVersionRequired() function as that is the real intent of the code in the constructor. I don't want to have this held up because of another diff though as i'm sure the people using a version less than 1.0.0 is close to 0% as they are on 1.9.0 right now.

src/lint/linter/ArcanistESLintLinter.php
14

This is the only way I see presently to set a min version. However as noted above the user can potential mess this up by setting a lower version in .arclint or .arcconfig and I have no way to stop this presently.

@michaeljs1990 That code is for handling when there's any error in linter itself. The code I wrote there was simply to help with debugging linter installation issues. For us, our eslint config is based on airbnb's config and also uses react, so we needed to install a few other packages globally. If you didn't have those installed, the linter will blow up and you won't see anything in sublime.

bspoon added inline comments.
src/lint/linter/ArcanistESLintLinter.php
30

Hi! I tried using this linter, and configured it to use the eslint binary relative to my project (node_modules/.bin/eslint), and it wouldn't report the version (because I don't have eslint installed globally).

Maybe the line should be this, instead?

list($output) = execx('%C --version', $this->getExecutableCommand());

Thanks for the feedback. Cool to see people using it before it's merged upstream.

src/lint/linter/ArcanistESLintLinter.php
30

You are correct, sorry I missed that. Will push up a change for it shortly.

does the linter not look for .eslintrc in project root by default? I run arc lint and it seems to use the rules from the global installed eslint.

@ninacfgarcia I believe that is correct due to the way that arc unit runs it's tests. I will have to check if that is common between other linters but you can set eslint.eslintconfig in your .arclint or .arcconfig file so it uses the config that you want.

@ninacfgarcia I believe that is correct due to the way that arc unit runs it's tests. I will have to check if that is common between other linters but you can set eslint.eslintconfig in your .arclint or .arcconfig file so it uses the config that you want.

Ah, I had to dig to the ends of the earth (or the eslint docs), but in the end, adding "root: true" to the .eslintrc was the solution. So just a quirk of the pathing when arc linter runs.

Anyway to get this back on track? I don't think it's in danger of going stale but it would be really nice to get this into master as it seems to have a fairly large following now.

@joshuaspence as the blocking reviewer can you take another look? It seems like @michaeljs1990 has addressed all your requests.

I'd probably add a method to set the configuration for ESLint via --config.

Line 69.

You should add a getVersion method.

Line 29.

You should maybe allow an environment to be specified via the --env flag.

Line 65.

You should use a custom output format to simplify the parseLinterOutput method (specifically, you should use --format $FORMAT where $FORMAT is either checkstyle, jslint-xml, junit or tap... whichever gives the best output).

Line 62.

What others steps must be taken before upstream can accept?

Hi @michaeljs1990, thank you for the code here! I had a use for this so I applied this patch.

The linter as it is won't parse the actual error codes (rule string)s correctly, although they are given in the eslint output. The linter also doesn't use the getLintMessageSeverity method to get the severity. As a result the severity cannot be customized in the .arclint config file. However, the linter still indicates that severity can be customized through returning true to canCustomizeLintSeverities method.

I fixed both issues in P1911 on top of this diff. The new code parses the rule string correctly and generates the correct description without extra information. severity and severity.rules are now supported in the .arclint file. I likely broke the tests though.

Hope this helps.

What others steps must be taken before upstream can accept?

I've collected the current state of the lint world in T10038. There's no specific pathway forward right now, but that task has additional discussion.

Any further work being done on getting this merged?

As above, T10038 still describes the current state of the world. It is very unlikely that any pathway forward will emerge outside of T10038.

@jcowgar and anyone who's interested in using ESLint. Right now we are using the script-and-regex linter for this. It works great for us. The .arclint config is as following:

{
  "linters": {
    "eslint-regex-based": {
      "type": "script-and-regex",
      "include": "(\\.jsx?$)",
      "exclude": [],
      "script-and-regex.script": "sh -c '([ -e ./node_modules/.bin/eslint ]) && (./node_modules/.bin/eslint -f compact \"$0\" || true)'",
      "script-and-regex.regex": "/^(?P<file>.*): line (?P<line>[0-9]*), col (?P<char>[0-9]*), (?P<warning>Warning|Error) - (?P<message>.*) \\((?P<code>[a-z-]+)\\)$/m"
    }
  }
}

Note that this treat all linter outputs as warnings.

michaeljs1990 edited reviewers, added: ogonkov; removed: michaeljs1990.

@jcowgar and anyone who's interested in using ESLint. Right now we are using the script-and-regex linter for this. It works great for us. The .arclint config is as following:

{
  "linters": {
    "eslint-regex-based": {
      "type": "script-and-regex",
      "include": "(\\.jsx?$)",
      "exclude": [],
      "script-and-regex.script": "sh -c '([ -e ./node_modules/.bin/eslint ]) && (./node_modules/.bin/eslint -f compact \"$0\" || true)'",
      "script-and-regex.regex": "/^(?P<file>.*): line (?P<line>[0-9]*), col (?P<char>[0-9]*), (?P<warning>Warning|Error) - (?P<message>.*) \\((?P<code>[a-z-]+)\\)$/m"
    }
  }
}

Note that this treat all linter outputs as warnings.

Here's my slight modification to split out warnings and errors:

{
  "linters": {
    "eslint-regex-based": {
      "type": "script-and-regex",
      "include": "(\\.jsx?$)",
      "exclude": [],
      "script-and-regex.script": "sh -c '([ -e ./node_modules/.bin/eslint ]) && (./node_modules/.bin/eslint -f compact \"$0\" || true)'",
      "script-and-regex.regex": "/^(?P<file>.*): line (?P<line>[0-9]*), col (?P<char>[0-9]*), ((?P<warning>Warning)|(?P<error>Error)) - (?P<message>.*) \\((?P<code>[a-z-\\/]+)\\)$/m"
    }
  }
}