diff --git a/src/lint/renderer/ArcanistCheckstyleXMLLintRenderer.php b/src/lint/renderer/ArcanistCheckstyleXMLLintRenderer.php index 32091bf5..7e43ec10 100644 --- a/src/lint/renderer/ArcanistCheckstyleXMLLintRenderer.php +++ b/src/lint/renderer/ArcanistCheckstyleXMLLintRenderer.php @@ -1,10 +1,9 @@ writer->setIndentString(' '); } - public function renderPreamble() { + public function willRenderResults() { $this->writer->startDocument('1.0', 'UTF-8'); $this->writer->startElement('checkstyle'); $this->writer->writeAttribute('version', '4.3'); - return $this->writer->flush(); + $this->writeOut($this->writer->flush()); } public function renderLintResult(ArcanistLintResult $result) { @@ -39,17 +38,13 @@ final class ArcanistCheckstyleXMLLintRenderer extends ArcanistLintRenderer { } $this->writer->endElement(); - return $this->writer->flush(); + $this->writeOut($this->writer->flush()); } - public function renderOkayResult() { - return ''; - } - - public function renderPostamble() { + public function didRenderResults() { $this->writer->endElement(); $this->writer->endDocument(); - return $this->writer->flush(); + $this->writeOut($this->writer->flush()); } private function getStringForSeverity($severity) { diff --git a/src/lint/renderer/ArcanistCompilerLintRenderer.php b/src/lint/renderer/ArcanistCompilerLintRenderer.php index 264750b1..947274d4 100644 --- a/src/lint/renderer/ArcanistCompilerLintRenderer.php +++ b/src/lint/renderer/ArcanistCompilerLintRenderer.php @@ -1,10 +1,9 @@ getMessages(); @@ -25,11 +24,7 @@ final class ArcanistCompilerLintRenderer extends ArcanistLintRenderer { $description); } - return implode('', $lines); - } - - public function renderOkayResult() { - return ''; + $this->writeOut(implode('', $lines)); } } diff --git a/src/lint/renderer/ArcanistConsoleLintRenderer.php b/src/lint/renderer/ArcanistConsoleLintRenderer.php index ea233e3d..fb90b590 100644 --- a/src/lint/renderer/ArcanistConsoleLintRenderer.php +++ b/src/lint/renderer/ArcanistConsoleLintRenderer.php @@ -1,17 +1,10 @@ showAutofixPatches = $show_autofix_patches; - return $this; - } + private $testableMode; public function setTestableMode($testable_mode) { $this->testableMode = $testable_mode; @@ -22,6 +15,38 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { return $this->testableMode; } + public function supportsPatching() { + return true; + } + + public function renderResultCode($result_code) { + if ($result_code == ArcanistLintWorkflow::RESULT_OKAY) { + $view = new PhutilConsoleInfo( + pht('OKAY'), + pht('No lint messages.')); + $this->writeOut($view->drawConsoleString()); + } + } + + public function promptForPatch( + ArcanistLintResult $result, + $old_path, + $new_path) { + + if ($old_path === null) { + $old_path = '/dev/null'; + } + + list($err, $stdout) = exec_manual('diff -u %s %s', $old_path, $new_path); + $this->writeOut($stdout); + + $prompt = pht( + 'Apply this patch to %s?', + tsprintf('__%s__', $result->getPath())); + + return phutil_console_confirm($prompt, $default_no = false); + } + public function renderLintResult(ArcanistLintResult $result) { $messages = $result->getMessages(); $path = $result->getPath(); @@ -31,10 +56,6 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { $text = array(); foreach ($messages as $message) { - if (!$this->showAutofixPatches && $message->isAutofix()) { - continue; - } - if ($message->isError()) { $color = 'red'; } else { @@ -77,9 +98,7 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { pht( 'Lint for %s:', phutil_console_format('__%s__', $path))); - return $prefix.implode("\n", $text); - } else { - return null; + $this->writeOut($prefix.implode("\n", $text)); } } @@ -278,13 +297,6 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { $data); } - public function renderOkayResult() { - return phutil_console_format( - "** %s ** %s\n", - pht('OKAY'), - pht('No lint warnings.')); - } - private function newOffsetMap($data) { $lines = phutil_split_lines($data); diff --git a/src/lint/renderer/ArcanistJSONLintRenderer.php b/src/lint/renderer/ArcanistJSONLintRenderer.php index 04432344..034ade8d 100644 --- a/src/lint/renderer/ArcanistJSONLintRenderer.php +++ b/src/lint/renderer/ArcanistJSONLintRenderer.php @@ -1,10 +1,9 @@ writeOut(json_encode($output)."\n"); } } diff --git a/src/lint/renderer/ArcanistLintRenderer.php b/src/lint/renderer/ArcanistLintRenderer.php index f153967d..1999fa6f 100644 --- a/src/lint/renderer/ArcanistLintRenderer.php +++ b/src/lint/renderer/ArcanistLintRenderer.php @@ -1,19 +1,61 @@ getPhobjectClassConstant('RENDERERKEY'); + } + + final public static function getAllRenderers() { + return id(new PhutilClassMapQuery()) + ->setAncestorClass(__CLASS__) + ->setUniqueMethod('getRendererKey') + ->execute(); + } + + final public function setOutputPath($path) { + $this->output = $path; + return $this; + } + + + /** + * Does this renderer support applying lint patches? + * + * @return bool True if patches should be applied when using this renderer. + */ + public function supportsPatching() { + return false; + } + + public function willRenderResults() { + return null; + } + + public function didRenderResults() { + return null; + } + + public function renderResultCode($result_code) { + return null; + } + + public function handleException(Exception $ex) { + throw $ex; } abstract public function renderLintResult(ArcanistLintResult $result); - abstract public function renderOkayResult(); - public function renderPostamble() { - return ''; + protected function writeOut($message) { + if ($this->output) { + Filesystem::appendFile($this->output, $message); + } else { + echo $message; + } + + return $this; } } diff --git a/src/lint/renderer/ArcanistNoneLintRenderer.php b/src/lint/renderer/ArcanistNoneLintRenderer.php index ce74e395..c28e2cb0 100644 --- a/src/lint/renderer/ArcanistNoneLintRenderer.php +++ b/src/lint/renderer/ArcanistNoneLintRenderer.php @@ -2,12 +2,10 @@ final class ArcanistNoneLintRenderer extends ArcanistLintRenderer { - public function renderLintResult(ArcanistLintResult $result) { - return ''; - } + const RENDERERKEY = 'none'; - public function renderOkayResult() { - return ''; + public function renderLintResult(ArcanistLintResult $result) { + return null; } } diff --git a/src/lint/renderer/ArcanistSummaryLintRenderer.php b/src/lint/renderer/ArcanistSummaryLintRenderer.php index 92eed3a4..22c6aba0 100644 --- a/src/lint/renderer/ArcanistSummaryLintRenderer.php +++ b/src/lint/renderer/ArcanistSummaryLintRenderer.php @@ -1,10 +1,9 @@ getMessages(); $path = $result->getPath(); @@ -19,14 +18,16 @@ final class ArcanistSummaryLintRenderer extends ArcanistLintRenderer { $text[] = "{$path}:{$line}:{$severity}: {$name}\n"; } - return implode('', $text); + $this->writeOut(implode('', $text)); } - public function renderOkayResult() { - return phutil_console_format( - "** %s ** %s\n", - pht('OKAY'), - pht('No lint warnings.')); + public function renderResultCode($result_code) { + if ($result_code == ArcanistLintWorkflow::RESULT_OKAY) { + $view = new PhutilConsoleInfo( + pht('OKAY'), + pht('No lint messages.')); + $this->writeOut($view->drawConsoleString()); + } } } diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php index 9014288b..680a601e 100644 --- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php +++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php @@ -171,7 +171,13 @@ EOTEXT; try { PhutilConsoleFormatter::disableANSI(true); - $actual = $renderer->renderLintResult($result); + + $tmp = new TempFile(); + $renderer->setOutputPath($tmp); + $renderer->renderLintResult($result); + $actual = Filesystem::readFile($tmp); + unset($tmp); + PhutilConsoleFormatter::disableANSI(false); } catch (Exception $ex) { PhutilConsoleFormatter::disableANSI(false); diff --git a/src/workflow/ArcanistLintWorkflow.php b/src/workflow/ArcanistLintWorkflow.php index b86cc6f7..16985bbb 100644 --- a/src/workflow/ArcanistLintWorkflow.php +++ b/src/workflow/ArcanistLintWorkflow.php @@ -74,12 +74,7 @@ EOTEXT ), 'output' => array( 'param' => 'format', - 'help' => pht( - "With 'summary', show lint warnings in a more compact format. ". - "With 'json', show lint warnings in machine-readable JSON format. ". - "With 'none', show no lint warnings. ". - "With 'compiler', show lint warnings in suitable for your editor. ". - "With 'xml', show lint warnings in the Checkstyle XML format."), + 'help' => pht('Select an output format.'), ), 'outfile' => array( 'param' => 'path', @@ -243,11 +238,8 @@ EOTEXT } if ($this->getArgument('amend-autofixes')) { - $prompt_autofix_patches = false; $this->shouldAmendChanges = true; $this->shouldAmendAutofixesWithoutPrompt = true; - } else { - $prompt_autofix_patches = true; } $repository_api = $this->getRepositoryAPI(); @@ -258,110 +250,76 @@ EOTEXT $wrote_to_disk = false; - switch ($this->getArgument('output')) { - case 'json': - $renderer = new ArcanistJSONLintRenderer(); - $prompt_patches = false; - $apply_patches = $this->getArgument('apply-patches'); - break; - case 'summary': - $renderer = new ArcanistSummaryLintRenderer(); - break; - case 'none': - $prompt_patches = false; - $apply_patches = $this->getArgument('apply-patches'); - $renderer = new ArcanistNoneLintRenderer(); - break; - case 'compiler': - $renderer = new ArcanistCompilerLintRenderer(); - $prompt_patches = false; - $apply_patches = $this->getArgument('apply-patches'); - break; - case 'xml': - $renderer = new ArcanistCheckstyleXMLLintRenderer(); - $prompt_patches = false; - $apply_patches = $this->getArgument('apply-patches'); - break; - default: - $renderer = new ArcanistConsoleLintRenderer(); - $renderer->setShowAutofixPatches($prompt_autofix_patches); - break; + $default_renderer = ArcanistConsoleLintRenderer::RENDERERKEY; + $renderer_key = $this->getArgument('output', $default_renderer); + + $renderers = ArcanistLintRenderer::getAllRenderers(); + if (!isset($renderers[$renderer_key])) { + throw new Exception( + pht( + 'Lint renderer "%s" is unknown. Supported renderers are: %s.', + $renderer_key, + implode(', ', array_keys($renderers)))); } + $renderer = $renderers[$renderer_key]; $all_autofix = true; - $tmp = null; - if ($this->getArgument('outfile') !== null) { - $tmp = id(new TempFile()) - ->setPreserveFile(true); - } - - $preamble = $renderer->renderPreamble(); - if ($tmp) { - Filesystem::appendFile($tmp, $preamble); + $out_path = $this->getArgument('outfile'); + if ($out_path !== null) { + $tmp = new TempFile(); + $renderer->setOutputPath((string)$tmp); } else { - $console->writeOut('%s', $preamble); + $tmp = null; } - foreach ($results as $result) { - $result_all_autofix = $result->isAllAutofix(); + if ($failed) { + $renderer->handleException($failed); + } - if (!$result->getMessages() && !$result_all_autofix) { + $renderer->willRenderResults(); + + $should_patch = ($apply_patches && $renderer->supportsPatching()); + foreach ($results as $result) { + if (!$result->getMessages()) { continue; } + $result_all_autofix = $result->isAllAutofix(); if (!$result_all_autofix) { $all_autofix = false; } - $lint_result = $renderer->renderLintResult($result); - if ($lint_result) { - if ($tmp) { - Filesystem::appendFile($tmp, $lint_result); - } else { - $console->writeOut('%s', $lint_result); - } - } + $renderer->renderLintResult($result); - if ($apply_patches && $result->isPatchable()) { + if ($should_patch && $result->isPatchable()) { $patcher = ArcanistLintPatcher::newFromArcanistLintResult($result); - $old_file = $result->getFilePathOnDisk(); - if ($prompt_patches && - !($result_all_autofix && !$prompt_autofix_patches)) { + $apply = true; + if ($prompt_patches && !$result_all_autofix) { + $old_file = $result->getFilePathOnDisk(); if (!Filesystem::pathExists($old_file)) { - $old_file = '/dev/null'; + $old_file = null; } + $new_file = new TempFile(); $new = $patcher->getModifiedFileContent(); Filesystem::writeFile($new_file, $new); - // TODO: Improve the behavior here, make it more like - // difference_render(). - list(, $stdout, $stderr) = - exec_manual('diff -u %s %s', $old_file, $new_file); - $console->writeOut('%s', $stdout); - $console->writeErr('%s', $stderr); - - $prompt = pht( - 'Apply this patch to %s?', - phutil_console_format('__%s__', $result->getPath())); - if (!phutil_console_confirm($prompt, $default_no = false)) { - continue; - } + $apply = $renderer->promptForPatch($result, $old_file, $new_file); } - $patcher->writePatchToDisk(); - $wrote_to_disk = true; + if ($apply) { + $patcher->writePatchToDisk(); + $wrote_to_disk = true; + } } } - $postamble = $renderer->renderPostamble(); + $renderer->didRenderResults(); + if ($tmp) { - Filesystem::appendFile($tmp, $postamble); - Filesystem::rename($tmp, $this->getArgument('outfile')); - } else { - $console->writeOut('%s', $postamble); + Filesystem::rename($tmp, $out_path); } if ($wrote_to_disk && $this->shouldAmendChanges) { @@ -391,20 +349,6 @@ EOTEXT } } - if ($this->getArgument('output') == 'json') { - // NOTE: Required by save_lint.php in Phabricator. - return 0; - } - - if ($failed) { - if ($failed instanceof ArcanistNoEffectException) { - if ($renderer instanceof ArcanistNoneLintRenderer) { - return 0; - } - } - throw $failed; - } - $unresolved = array(); $has_warnings = false; $has_errors = false; @@ -433,11 +377,7 @@ EOTEXT $result_code = self::RESULT_OKAY; } - if (!$this->getParentWorkflow()) { - if ($result_code == self::RESULT_OKAY) { - $console->writeOut('%s', $renderer->renderOkayResult()); - } - } + $renderer->renderResultCode($result_code); return $result_code; }