Some small fixes

[kuba-orlik]

1. Prettier linter now doesn't show a warning for files that don't require any formatting changes (are already formatted)
2. ESLintLinter and PrettierLinter now don't throw an error because of return status code 1
3. Fix ESLinter failing because of missing source parameter in some of messages in eslint output

[CrabDude]

ESLintLinter and PrettierLinter now don't throw an error because of return status code 1 Can you help me understand better why this is desirable? For instance, it suppresses the failure when ./node_modules/* dne (i.e., when --cwd is set or the de facto binary has not been installed).

Is there a need for the wrapper scripts outside of exit code suppression? If not, we would should remove them. That said, your commit definitely exposes some improvements to PrettierEslintLinter.php that were never back-ported so to speak to EslintLinter.php & PrettierLinter.php like the getDefaultBinary() lookup.

Note: The only reason for the node4_proxy wrapper existing was b/c it was the only way to support node 4 and it's need for the --harmony_array_includes feature flag utilized in the prettier-eslint-cli source. Since neither eslint nor prettier have that same need, we would want to avoid a wrapper script to ensure linter logic is consolidated.

[kuba-orlik]

Could you elaborate on the reason for adding the wrapper scripts?

Sure. When eslint encounters a lint issue with "error" severity, it returns the normal JSON output to stdout, but exits with exit code 1. This causes Arcanist to assume that something went badly:

 Exception 
Some linters failed:
    - CommandException: Command failed with error #1!
      COMMAND
      '/home/kuba/projects/sealcode/project/.pinterest-linters/src/.eslint-zero.sh' '--format=json' '--no-color' '/home/kuba/projects/sealcode/project/config.js'

      STDOUT
      [{"filePath":"/home/kuba/projects/sealcode/project/config.js","messages":[{"ruleId":"no-console","severity":1,"message":"Unexpected console statement.","line":1,"column":1,"nodeType":"MemberExpression","messageId":"unexpected","endLine":1,"endColumn":12},{"ruleId":"no-unused-vars","severity":2,"message":"'a' is assigned a value but never used.","line":3,"column":7,"nodeType":"Identifier","endLine":3,"endColumn":8}],"errorCount":1,"warningCount":1,"fixableErrorCount":0,"fixableWarningCount":0,"source":"console.log(\"HELP ME\");\n\nconst a = 0;\n\nmodule.exports = {\n\tlogger: {\n\t\tlevel: process.env.SEALIOUS_LOG_LEVEL || \"info\",\n\t},\n\tupload_path: \"/tmp\",\n\tdatastore_mongo: {\n\t\thost: \"db\",\n\t\tdb_name: \"sealious\",\n\t},\n\timage_formats: {\n\t\tmedium: {\n\t\t\tsize: [300, 180],\n\t\t},\n\t\tsmall: {\n\t\t\tsize: [100, 100],\n\t\t},\n\t},\n\t\"www-server\": {\n\t\tport: 8082,\n\t},\n\tsmtp: {\n\t\thost: \"mailcatcher\",\n\t\tport: 1025,\n\t\tuser: \"any\",\n\t\tpassword: ... (280 more bytes) ...

      STDERR
      (empty)
(Run with `--trace` for a full exception trace.)

Is there a need for the wrapper scripts outside of exit code suppression? If not, we would should remove them.

No, exit code suspension was my only motivation here.

[jparise]

Sure. When eslint encounters a lint issue with "error" severity, it returns the normal JSON output to stdout, but exits with exit code 1.

Ah. You can tell Arcanist that a non-zero exit is normal:

  /**
   * Return true to continue when the external linter exits with an error code.
   * By default, linters which exit with an error code are assumed to have
   * failed. However, some linters exit with a specific code to indicate that
   * lint messages were detected.
   *
   * If the linter sometimes raises errors during normal operation, override
   * this method and return true so execution continues when it exits with
   * a nonzero status.
   *
   * @param bool  Return true to continue on nonzero error code.
   * @task bin
   */
  public function shouldExpectCommandErrors() {
    return true;
  }

The default value changed from false to true in 2015 in D11322. Are you perhaps running an old Arcanist installation?

We can also add an explicit shouldExpectCommandErrors implementation to these linters that returns true.

[kuba-orlik]

Are you perhaps running an old Arcanist installation?

I'm on the newest version:

~ >>> arc version                                                              
arcanist d9a4293ae734756823b4a3ca202f185c57f3e834 (3 Aug 2018)
libphutil 5b341cc09ca9bb707be469c7f23bbf6a961bc593 (15 Aug 2018)

[CrabDude]

Closing in favor of https://github.com/pinterest/arcanist-linters/pull/8. (Still needs testing, but wanted to move the conversation forward)

@kuba-orlik If you'd like, feel free to submit a new PR with just the #1 & #3 fixes from this commit, and I'll land mine on top.

[kuba-orlik]

Thanks for picking it up! As #8 already contains those fixes, I think it'd be best to just keep them there :)