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.

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.

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

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

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.

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

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

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

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

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.

In D12198#165635, @michaeljs1990 wrote:

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

ping, would love to use this

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.

Merge with JSON parsing from D15124

In D12198#181207, @chengyin wrote:

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