pinterest / arcanist-linters

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

Fix setName call in ESLintLinter #24

Closed jhurwitz closed 4 years ago

jhurwitz commented 4 years ago

Fixes a bug introduced in #22.

$offense['ruleId'] || 'unknown' was returning a bool (true) which caused the following exception when arc tried to post the lint results via the harbormaster.sendmessage API:

[2019-10-15 01:34:16] EXCEPTION: (ConduitClientException) ERR-CONDUIT-CORE: Parameter 'name' has invalid type. Expected type 'string', got type 'bool'. at [<phutil>/src/conduit/ConduitFuture.php:62]
arcanist(head=master, ref.master=96fde137a1dc), linters(head=test, ref.master=81e8d886d5c8, ref.test=f20a8b42a357), phutil(head=master, ref.master=f434f57578dd)
  #0 <#2> ConduitFuture::didReceiveResult(array) called at [<phutil>/src/future/FutureProxy.php:58]
  #1 <#2> FutureProxy::getResult() called at [<phutil>/src/future/FutureProxy.php:35]
  #2 <#2> FutureProxy::resolve() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:2999]
  #3 phlog(ConduitClientException) called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:3005]
  #4 ArcanistDiffWorkflow::updateAutotargets(string, NULL) called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:537]
  #5 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:394]

In these cases, arc lint and arc diff would successfully run, but the diff created on Phabricator would not show any details about what the lint errors/warnings were, or what line numbers they were on.

With this fix, you can once again see the details of the lint errors and warnings in the Differential UI on Phabricator.

I also changed the default name (when $offense['ruleId'] is not defined) from unknown to ESLintLinter, and tested both cases: https://www.dropbox.com/s/0mb54bf5xfe34hi/Screenshot%202019-10-14%2019.20.51.png?dl=0

jparise commented 4 years ago

@jhurwitz I just merged #25, which might also solve this issue.

jparise commented 4 years ago

I'm going to close this in favor of the solution implemented in #25.