-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add PrettierESLintLinter #4
Conversation
Could we instead adjust the relative priorities of these two linters so they always run in a predictable order? |
src/PrettierESLintLinter.php
Outdated
echo $this->getProjectRoot() . '/' . $this->cwd; | ||
list($err, $stdout, $stderr) = exec_manual('yarn -s --cwd %s which prettier-eslint', $this->getProjectRoot() . '/' . $this->cwd); | ||
$binaryPath = strtok($stdout, "\n"); | ||
echo $binaryPath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debugging echo
src/PrettierESLintLinter.php
Outdated
} | ||
|
||
public function getDefaultInterpreter() { | ||
return __DIR__ . '/node4_proxy'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this just be node
with --harmony_array_includes
as a mandatory flag?
I don't love shipping the helper script.
@@ -11,8 +11,8 @@ | |||
'class' => array( | |||
'ApacheThriftGeneratedLinter' => 'src/ApacheThriftGeneratedLinter.php', | |||
'ApacheThriftLinter' => 'src/ApacheThriftLinter.php', | |||
'PrettierLinter' => 'src/PrettierLinter.php', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed PrettierLinter
by mistake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed & sorted.
1c1150a
to
a8b0340
Compare
97311a7
to
989ad4f
Compare
This is good to go. Includes 2 edge case fixes:
Includes 2 improvements:
|
src/PrettierESLintLinter.php
Outdated
public function getDefaultBinary() { | ||
list($err, $stdout, $stderr) = exec_manual('yarn -s --cwd %s which prettier-eslint', $this->getProjectRoot() . '/' . $this->cwd); | ||
$binaryPath = strtok($stdout, "\n"); | ||
if ($binaryPath == "") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (empty($binaryPath)) {
is a little more idiomatic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/PrettierESLintLinter.php
Outdated
$originalText = $this->getData($path); | ||
$messages = array(); | ||
|
||
// Ignored files have empty $stdout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"having" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarified.
// Note: $stdout is empty for ignored files
`$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 <pinterest#2> ConduitFuture::didReceiveResult(array) called at [<phutil>/src/future/FutureProxy.php:58] pinterest#1 <pinterest#2> FutureProxy::getResult() called at [<phutil>/src/future/FutureProxy.php:35] pinterest#2 <pinterest#2> FutureProxy::resolve() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:2999] pinterest#3 phlog(ConduitClientException) called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:3005] pinterest#4 ArcanistDiffWorkflow::updateAutotargets(string, NULL) called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:537] pinterest#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
No description provided.