Page MenuHomePhabricator

Added ESLint Support
Changes PlannedPublic

Authored by michaeljs1990 on Mar 29 2015, 7:01 AM.
Tags
None
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
Lint
Lint OK
Unit
Unit Test Errors
Build Status
Buildable 12962
Build 17339: Run Core Tests
Build 16548: 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!

michaeljs1990 added a comment.EditedAug 2 2015, 9:45 PM

@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.

adamchainz added a subscriber: adamchainz.
oluckyman added a subscriber: oluckyman.

@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.

Updated based on review from @joshuaspence

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
11

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
2

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.

michaeljs1990 added a comment.EditedNov 13 2015, 7:33 PM

@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.

avivey added a subscriber: avivey.Nov 14 2015, 8:46 PM
  • 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
20

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

👍

src/lint/linter/__tests__/ArcanistESLintLinterTestCase.php
11

what do all other lint test cases do?

avivey updated this object.Nov 14 2015, 8:46 PM

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
11

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

michaeljs1990 marked 3 inline comments as done.Nov 15 2015, 10:17 PM

@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.

I was thinking about T4954 and D9161.

  • 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
15

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.

michaeljs1990 edited the test plan for this revision. (Show Details)Nov 16 2015, 7:25 PM
bspoon added a subscriber: bspoon.Nov 19 2015, 8:33 PM
bspoon added inline comments.
src/lint/linter/ArcanistESLintLinter.php
31

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
31

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

ninacfgarcia added a subscriber: ninacfgarcia.EditedDec 1 2015, 4:25 PM

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.

ox added a subscriber: ox.Dec 4 2015, 7:39 PM
turadg added a comment.EditedDec 12 2015, 12:29 AM

@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?

ping, would love to use this

cwang added a subscriber: cwang.

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.

yelirekim awarded a token.
jcowgar added a subscriber: jcowgar.Apr 9 2016, 6:55 PM

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.

turadg removed a subscriber: turadg.Apr 9 2016, 7:30 PM
ogonkov commandeered this revision.Jul 7 2016, 10:06 AM
ogonkov added a reviewer: michaeljs1990.
ogonkov updated this revision to Diff 39086.Jul 7 2016, 10:23 AM
  • Parse JSON output

Merge with JSON parsing from D15124

simonft added a subscriber: simonft.Jul 8 2016, 5:58 PM
michaeljs1990 commandeered this revision.Aug 28 2016, 3:10 PM
michaeljs1990 edited reviewers, added: ogonkov; removed: michaeljs1990.
michaeljs1990 planned changes to this revision.Aug 28 2016, 3:10 PM
jcowgar removed a subscriber: jcowgar.Aug 28 2016, 3:35 PM
stevex added a subscriber: stevex.Sep 19 2016, 3:31 PM
sudowork added subscribers: jcowgar, sudowork.EditedMar 9 2017, 1:49 AM

@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"
    }
  }
}