From a7327ca0e9432bd14bdd5fe19c54d1a157e0c9a7 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Mon, 5 May 2014 14:22:26 -0700 Subject: [PATCH] Modernize `ArcanistJSHintLinter`. Summary: Modernize `ArcanistJSHintLinter` by extending from `ArcanistExternalLinter` instead of `ArcanistLinter`. Test Plan: Wrote and executed unit tests. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: epriestley, Korvin Differential Revision: https://secure.phabricator.com/D8965 --- src/__phutil_library_map__.php | 4 +- src/lint/linter/ArcanistJSHintLinter.php | 229 +++++++----------- .../ArcanistJSHintLinterTestCase.php | 12 + .../__tests__/jshint/dot-notation.lint-test | 6 + .../jshint/expected-conditional.lint-test | 6 + .../jshint/missing-semicolon.lint-test | 3 + .../jshint/unnecessary-semicolon.lint-test | 5 + src/lint/linter/reporter.js | 11 +- 8 files changed, 132 insertions(+), 144 deletions(-) create mode 100644 src/lint/linter/__tests__/ArcanistJSHintLinterTestCase.php create mode 100644 src/lint/linter/__tests__/jshint/dot-notation.lint-test create mode 100644 src/lint/linter/__tests__/jshint/expected-conditional.lint-test create mode 100644 src/lint/linter/__tests__/jshint/missing-semicolon.lint-test create mode 100644 src/lint/linter/__tests__/jshint/unnecessary-semicolon.lint-test diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index cc104691..9994911e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -83,6 +83,7 @@ phutil_register_library_map(array( 'ArcanistInlinesWorkflow' => 'workflow/ArcanistInlinesWorkflow.php', 'ArcanistInstallCertificateWorkflow' => 'workflow/ArcanistInstallCertificateWorkflow.php', 'ArcanistJSHintLinter' => 'lint/linter/ArcanistJSHintLinter.php', + 'ArcanistJSHintLinterTestCase' => 'lint/linter/__tests__/ArcanistJSHintLinterTestCase.php', 'ArcanistLandWorkflow' => 'workflow/ArcanistLandWorkflow.php', 'ArcanistLiberateWorkflow' => 'workflow/ArcanistLiberateWorkflow.php', 'ArcanistLintConsoleRenderer' => 'lint/renderer/ArcanistLintConsoleRenderer.php', @@ -241,7 +242,8 @@ phutil_register_library_map(array( 'ArcanistHgServerChannel' => 'PhutilProtocolChannel', 'ArcanistInlinesWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistInstallCertificateWorkflow' => 'ArcanistBaseWorkflow', - 'ArcanistJSHintLinter' => 'ArcanistLinter', + 'ArcanistJSHintLinter' => 'ArcanistExternalLinter', + 'ArcanistJSHintLinterTestCase' => 'ArcanistArcanistLinterTestCase', 'ArcanistLandWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistLiberateWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistLintConsoleRenderer' => 'ArcanistLintRenderer', diff --git a/src/lint/linter/ArcanistJSHintLinter.php b/src/lint/linter/ArcanistJSHintLinter.php index 9f5ddb15..ffd444f4 100644 --- a/src/lint/linter/ArcanistJSHintLinter.php +++ b/src/lint/linter/ArcanistJSHintLinter.php @@ -1,44 +1,11 @@ getEngine()->getConfigurationManager(); + $prefix = $config->getConfigFromAnySource('lint.jshint.prefix'); + $bin = $config->getConfigFromAnySource('lint.jshint.bin', 'jshint'); + + if ($prefix) { + return $prefix.'/'.$bin; + } else { + return $bin; + } + } + + public function getCacheVersion() { + list($stdout) = execx('%C --version', $this->getExecutableCommand()); + + // Extract version number from standard output. + $matches = array(); + if (preg_match('/^jshint v(\d+\.\d+\.\d+)$/', $stdout, $matches)) { + $version = $matches[1]; + } else { + $version = md5($stdout); + } + + return $version . '-' . md5(json_encode($this->getCommandFlags())); + } + + public function getInstallInstructions() { + return pht('Install JSHint using `npm install -g jshint`.'); + } + + public function shouldExpectCommandErrors() { + return true; + } + + public function supportsReadDataFromStdin() { + return true; + } + + public function getReadDataFromStdinFilename() { + return '-'; + } + + public function getMandatoryFlags() { return array( - self::JSHINT_ERROR => ArcanistLintSeverity::SEVERITY_ERROR + '--reporter='.dirname(realpath(__FILE__)).'/reporter.js', ); } - // placeholder if/until we get a map code -> name map - // jshint only offers code -> description right now (parsed as 'reason') - public function getLintMessageName($code) { - return "JSHint".$code; - } - - public function getLintNameMap() { - return array( - self::JSHINT_ERROR => "JSHint Error" - ); - } - - public function getJSHintOptions() { + public function getDefaultFlags() { $config_manager = $this->getEngine()->getConfigurationManager(); - $options = '--reporter '.dirname(realpath(__FILE__)).'/reporter.js'; + $options = $config_manager->getConfigFromAnySource( + 'lint.jshint.options', + array()); + $config = $config_manager->getConfigFromAnySource('lint.jshint.config'); - - $working_copy = $this->getEngine()->getWorkingCopy(); - if ($config !== null) { - $config = Filesystem::resolvePath( - $config, - $working_copy->getProjectRoot()); - - if (!Filesystem::pathExists($config)) { - throw new ArcanistUsageException( - "Unable to find custom options file defined by ". - "'lint.jshint.config'. Make sure that the path is correct."); - } - - $options .= ' --config '.$config; + if ($config) { + $options[] = '--config='.$config; } return $options; } - private function getJSHintPath() { - $config_manager = $this->getEngine()->getConfigurationManager(); - $prefix = $config_manager->getConfigFromAnySource('lint.jshint.prefix'); - $bin = $config_manager->getConfigFromAnySource('lint.jshint.bin'); - - if ($bin === null) { - $bin = "jshint"; - } - - if ($prefix !== null) { - $bin = $prefix."/".$bin; - - if (!Filesystem::pathExists($bin)) { - throw new ArcanistUsageException( - "Unable to find JSHint binary in a specified directory. Make sure ". - "that 'lint.jshint.prefix' and 'lint.jshint.bin' keys are set ". - "correctly. If you'd rather use a copy of JSHint installed ". - "globally, you can just remove these keys from your .arcconfig"); - } - - return $bin; - } - - if (!Filesystem::binaryExists($bin)) { - throw new ArcanistUsageException( - "JSHint does not appear to be installed on this system. Install it ". - "(e.g., with 'npm install jshint -g') or configure ". - "'lint.jshint.prefix' in your .arcconfig to point to the directory ". - "where it resides."); - } - - return $bin; - } - - public function willLintPaths(array $paths) { - if (!$this->isCodeEnabled(self::JSHINT_ERROR)) { - return; - } - - $jshint_bin = $this->getJSHintPath(); - $jshint_options = $this->getJSHintOptions(); - $futures = array(); - - foreach ($paths as $path) { - $filepath = $this->getEngine()->getFilePathOnDisk($path); - $futures[$path] = new ExecFuture( - "%s %s %C", - $jshint_bin, - $filepath, - $jshint_options); - } - - foreach (Futures($futures)->limit(8) as $path => $future) { - $this->results[$path] = $future->resolve(); - } - } - - public function lintPath($path) { - if (!$this->isCodeEnabled(self::JSHINT_ERROR)) { - return; - } - - list($rc, $stdout, $stderr) = $this->results[$path]; - - if ($rc === 0) { - return; - } - + protected function parseLinterOutput($path, $err, $stdout, $stderr) { $errors = json_decode($stdout); + if (!is_array($errors)) { // Something went wrong and we can't decode the output. Exit abnormally. throw new ArcanistUsageException( @@ -166,12 +96,35 @@ final class ArcanistJSHintLinter extends ArcanistLinter { "stderr:\n\n{$stderr}"); } + $messages = array(); foreach ($errors as $err) { - $this->raiseLintAtLine( - $err->line, - $err->col, - $err->code, - $err->reason); + $message = new ArcanistLintMessage(); + $message->setPath($path); + $message->setLine($err->line); + $message->setChar($err->col); + $message->setCode($err->code); + $message->setName('JSHint'.$err->code); + $message->setDescription($err->reason); + $message->setSeverity($this->getLintMessageSeverity($err->code)); + $message->setOriginalText($err->evidence); + + $messages[] = $message; } + + return $messages; + } + + protected function getLintCodeFromLinterConfigurationKey($code) { + if (!preg_match('/^(E|W)\d+$/', $code)) { + throw new Exception( + pht( + 'Unrecognized lint message code "%s". Expected a valid JSHint '. + 'lint code like "%s" or "%s".', + $code, + 'E033', + 'W093')); + } + + return $code; } } diff --git a/src/lint/linter/__tests__/ArcanistJSHintLinterTestCase.php b/src/lint/linter/__tests__/ArcanistJSHintLinterTestCase.php new file mode 100644 index 00000000..95405999 --- /dev/null +++ b/src/lint/linter/__tests__/ArcanistJSHintLinterTestCase.php @@ -0,0 +1,12 @@ +executeTestsInDirectory( + dirname(__FILE__).'/jshint/', + new ArcanistJSHintLinter()); + } + +} diff --git a/src/lint/linter/__tests__/jshint/dot-notation.lint-test b/src/lint/linter/__tests__/jshint/dot-notation.lint-test new file mode 100644 index 00000000..681e3505 --- /dev/null +++ b/src/lint/linter/__tests__/jshint/dot-notation.lint-test @@ -0,0 +1,6 @@ +var args = {}; +args['foo'] = 'bar'; +args['bar'] = 'baz'; +~~~~~~~~~~ +warning:2:5 +warning:3:5 diff --git a/src/lint/linter/__tests__/jshint/expected-conditional.lint-test b/src/lint/linter/__tests__/jshint/expected-conditional.lint-test new file mode 100644 index 00000000..2b229273 --- /dev/null +++ b/src/lint/linter/__tests__/jshint/expected-conditional.lint-test @@ -0,0 +1,6 @@ +var foo; +if (foo = 'bar') { + return true; +} +~~~~~~~~~~ +warning:2:16 diff --git a/src/lint/linter/__tests__/jshint/missing-semicolon.lint-test b/src/lint/linter/__tests__/jshint/missing-semicolon.lint-test new file mode 100644 index 00000000..a6669698 --- /dev/null +++ b/src/lint/linter/__tests__/jshint/missing-semicolon.lint-test @@ -0,0 +1,3 @@ +console.log('foobar') +~~~~~~~~~~ +warning:1:22 diff --git a/src/lint/linter/__tests__/jshint/unnecessary-semicolon.lint-test b/src/lint/linter/__tests__/jshint/unnecessary-semicolon.lint-test new file mode 100644 index 00000000..14eafbdf --- /dev/null +++ b/src/lint/linter/__tests__/jshint/unnecessary-semicolon.lint-test @@ -0,0 +1,5 @@ +function main() { + return 'Hello, World!'; +}; +~~~~~~~~~~ +warning:3:2 diff --git a/src/lint/linter/reporter.js b/src/lint/linter/reporter.js index 768d6393..e2d4ff5a 100644 --- a/src/lint/linter/reporter.js +++ b/src/lint/linter/reporter.js @@ -5,11 +5,12 @@ module.exports = { results.forEach(function (result) { var error = result.error; report.push({ - 'file' : result.file, - 'line' : error.line, - 'col' : error.character, - 'reason': error.reason, - 'code' : error.code, + 'file' : result.file, + 'line' : error.line, + 'col' : error.character, + 'reason' : error.reason, + 'code' : error.code, + 'evidence': error.evidence, }); });