diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -164,6 +164,7 @@ 'ArcanistRubyLinter' => 'lint/linter/ArcanistRubyLinter.php', 'ArcanistRubyLinterTestCase' => 'lint/linter/__tests__/ArcanistRubyLinterTestCase.php', 'ArcanistScriptAndRegexLinter' => 'lint/linter/ArcanistScriptAndRegexLinter.php', + 'ArcanistScriptAndRegexLinterTestCase' => 'lint/linter/__tests__/ArcanistScriptAndRegexLinterTestCase.php', 'ArcanistSetConfigWorkflow' => 'workflow/ArcanistSetConfigWorkflow.php', 'ArcanistSettings' => 'configuration/ArcanistSettings.php', 'ArcanistShellCompleteWorkflow' => 'workflow/ArcanistShellCompleteWorkflow.php', @@ -349,7 +350,8 @@ 'ArcanistRevertWorkflow' => 'ArcanistWorkflow', 'ArcanistRubyLinter' => 'ArcanistExternalLinter', 'ArcanistRubyLinterTestCase' => 'ArcanistExternalLinterTestCase', - 'ArcanistScriptAndRegexLinter' => 'ArcanistLinter', + 'ArcanistScriptAndRegexLinter' => 'ArcanistExternalLinter', + 'ArcanistScriptAndRegexLinterTestCase' => 'ArcanistExternalLinterTestCase', 'ArcanistSetConfigWorkflow' => 'ArcanistWorkflow', 'ArcanistShellCompleteWorkflow' => 'ArcanistWorkflow', 'ArcanistSingleLintEngine' => 'ArcanistLintEngine', diff --git a/src/lint/ArcanistLintSeverity.php b/src/lint/ArcanistLintSeverity.php --- a/src/lint/ArcanistLintSeverity.php +++ b/src/lint/ArcanistLintSeverity.php @@ -47,4 +47,19 @@ return $map[$message_sev] >= idx($map, $level, 0); } + /** + * Returns a mapping from human-readable string to linter severity. + * + * @return map + */ + public static function getSeverityMap() { + return array( + 'error' => self::SEVERITY_ERROR, + 'warning' => self::SEVERITY_WARNING, + 'autofix' => self::SEVERITY_AUTOFIX, + 'advice' => self::SEVERITY_ADVICE, + 'disabled' => self::SEVERITY_DISABLED, + ); + } + } diff --git a/src/lint/linter/ArcanistExternalLinter.php b/src/lint/linter/ArcanistExternalLinter.php --- a/src/lint/linter/ArcanistExternalLinter.php +++ b/src/lint/linter/ArcanistExternalLinter.php @@ -259,6 +259,10 @@ $binary = $this->getBinary(); + if ($binary === null) { + return; + } + // NOTE: If we have an interpreter, we don't require the script to be // executable (so we just check that the path exists). Otherwise, the // binary must be executable. diff --git a/src/lint/linter/ArcanistLinter.php b/src/lint/linter/ArcanistLinter.php --- a/src/lint/linter/ArcanistLinter.php +++ b/src/lint/linter/ArcanistLinter.php @@ -366,13 +366,7 @@ } public function setLinterConfigurationValue($key, $value) { - $sev_map = array( - 'error' => ArcanistLintSeverity::SEVERITY_ERROR, - 'warning' => ArcanistLintSeverity::SEVERITY_WARNING, - 'autofix' => ArcanistLintSeverity::SEVERITY_AUTOFIX, - 'advice' => ArcanistLintSeverity::SEVERITY_ADVICE, - 'disabled' => ArcanistLintSeverity::SEVERITY_DISABLED, - ); + $sev_map = ArcanistLintSeverity::getSeverityMap(); switch ($key) { case 'severity': diff --git a/src/lint/linter/ArcanistScriptAndRegexLinter.php b/src/lint/linter/ArcanistScriptAndRegexLinter.php --- a/src/lint/linter/ArcanistScriptAndRegexLinter.php +++ b/src/lint/linter/ArcanistScriptAndRegexLinter.php @@ -8,11 +8,11 @@ * * Configure this linter by setting these keys in your configuration: * - * - `linter.scriptandregex.script` Script command to run. This can be - * the path to a linter script, but may also include flags or use shell - * features (see below for examples). - * - `linter.scriptandregex.regex` The regex to process output with. This - * regex uses named capturing groups (detailed below) to interpret output. + * - `script-and-regex.script`: Script command to run. This can be the path to + * a linter script, but may also include flags or use shell features (see + * below for examples). + * - `script-and-regex.regex`: The regex to process output with. This regex + * uses named capturing groups (detailed below) to interpret output. * * The script will be invoked from the project root, so you can specify a * relative path like `scripts/lint.sh` or an absolute path like @@ -22,35 +22,35 @@ * linter which can perform custom processing, but may be somewhat simpler to * configure. * - * == Script... == + * == Script == * * The script will be invoked once for each file that is to be linted, with - * the file passed as the first argument. The file may begin with a "-"; ensure + * the file passed as the first argument. The file may begin with a `-`; ensure * your script will not interpret such files as flags (perhaps by ending your - * script configuration with "--", if its argument parser supports that). + * script configuration with `--`, if its argument parser supports that). * * Note that when run via `arc diff`, the list of files to be linted includes - * deleted files and files that were moved away by the change. The linter should - * not assume the path it is given exists, and it is not an error for the - * linter to be invoked with paths which are no longer there. (Every affected - * path is subject to lint because some linters may raise errors in other files - * when a file is removed, or raise an error about its removal.) + * deleted files and files that were moved away by the change. The linter + * should not assume the path it is given exists, and it is not an error for + * the linter to be invoked with paths which are no longer there. (Every + * affected path is subject to lint because some linters may raise errors in + * other files when a file is removed, or raise an error about its removal). * - * The script should emit lint messages to stdout, which will be parsed with + * The script should emit lint messages to `stdout`, which will be parsed with * the provided regex. * * For example, you might use a configuration like this: * * /opt/lint/lint.sh --flag value --other-flag -- * - * stderr is ignored. If you have a script which writes messages to stderr, - * you can redirect stderr to stdout by using a configuration like this: + * `stderr` is ignored. If you have a script which writes messages to `stderr`, + * you can redirect `stderr` to `stdout` by using a configuration like this: * * sh -c '/opt/lint/lint.sh "$0" 2>&1' * - * The return code of the script must be 0, or an exception will be raised + * The return code of the script must be `0`, or an exception will be raised * reporting that the linter failed. If you have a script which exits nonzero - * under normal circumstances, you can force it to always exit 0 by using a + * under normal circumstances, you can force it to always exit `0` by using a * configuration like this: * * sh -c '/opt/lint/lint.sh "$0" || true' @@ -63,13 +63,13 @@ * COUNTEREXAMPLE * sh -c '/opt/lint/lint.sh --output /tmp/lint.out "$0" && cat /tmp/lint.out' * - * There are necessary limits to how gracefully this linter can deal with - * edge cases, because it is just a script and a regex. If you need to do - * things that this linter can't handle, you can write a phutil linter and move - * the logic to handle those cases into PHP. PHP is a better general-purpose + * There are necessary limits to how gracefully this linter can deal with edge + * cases, because it is just a script and a regex. If you need to do things + * that this linter can't handle, you can write a phutil linter and move the + * logic to handle those cases into PHP. PHP is a better general-purpose * programming language than regular expressions are, if only by a small margin. * - * == ...and Regex == + * == Regex == * * The regex must be a valid PHP PCRE regex, including delimiters and flags. * @@ -90,10 +90,10 @@ * `disabled`. These allow you to match output formats like "E123" and * "W123" to indicate errors and warnings, even though the word "error" is * not present in the output. If no severity capturing group is present, - * messages are raised with "error" severity. If multiple severity capturing - * groups are present, messages are raised with the highest captured - * severity. Capturing groups like `error` supersede the `severity` - * capturing group. + * messages are raised with "error" severity. If multiple severity + * capturing groups are present, messages are raised with the highest + * captured severity. Capturing groups like `error` supersede the + * `severity` capturing group. * - `error` (optional) Match some nonempty substring to indicate that this * message has "error" severity. * - `warning` (optional) Match some nonempty substring to indicate that this @@ -153,11 +153,10 @@ * @task parse Parsing Output * @task config Validating Configuration */ -final class ArcanistScriptAndRegexLinter extends ArcanistLinter { +final class ArcanistScriptAndRegexLinter extends ArcanistExternalLinter { private $script = null; private $regex = null; - private $output = array(); public function getInfoName() { return pht('Script and Regex'); @@ -170,49 +169,90 @@ 'run custom lint scripts.'); } -/* -( Linting )------------------------------------------------------------ */ + public function getLinterName() { + return 'S&RX'; + } + public function getLinterConfigurationName() { + return 'script-and-regex'; + } - /** - * Run the script on each file to be linted. - * - * @task lint - */ - public function willLintPaths(array $paths) { - $script = $this->getConfiguredScript(); - $root = $this->getEngine()->getWorkingCopy()->getProjectRoot(); - - $futures = array(); - foreach ($paths as $path) { - $future = new ExecFuture('%C %s', $script, $path); - $future->setCWD($root); - $futures[$path] = $future; - } + public function getLinterConfigurationOptions() { + // These fields are optional only to avoid breaking things. + $options = array( + 'script-and-regex.script' => array( + 'type' => 'optional string', + 'help' => pht('Script to execute.'), + ), + 'script-and-regex.regex' => array( + 'type' => 'optional regex', + 'help' => pht('The regex to process output with.'), + ), + ); - $futures = id(new FutureIterator($futures)) - ->limit(4); - foreach ($futures as $path => $future) { - list($stdout) = $future->resolvex(); - $this->output[$path] = $stdout; + return $options + parent::getLinterConfigurationOptions(); + } + + public function setLinterConfigurationValue($key, $value) { + switch ($key) { + case 'script-and-regex.script': + $this->script = $value; + return; + case 'script-and-regex.regex': + $this->regex = $value; + return; + + default: + return parent::setLinterConfigurationValue($key, $value); } } + public function getDefaultBinary() { + return $this->script; + } + + public function getInstallInstructions() { + return null; + } + + public function shouldExpectCommandErrors() { + return false; + } + + public function supportsReadDataFromStdin() { + return false; + } + + public function getReadDataFromStdinFilename() { + return null; + } + + protected function getMandatoryFlags() { + return array(); + } + + protected function getDefaultFlags() { + return array(); + } + + +/* -( Linting )------------------------------------------------------------ */ + /** - * Run the regex on the output of the script. + * Run the script on each file to be linted. * * @task lint */ - public function lintPath($path) { + protected function parseLinterOutput($path, $err, $stdout, $stderr) { $regex = $this->getConfiguredRegex(); - $output = idx($this->output, $path); - if (!strlen($output)) { + if (!strlen($stdout)) { // No output, but it exited 0, so just move on. return; } $matches = null; - if (!preg_match_all($regex, $output, $matches, PREG_SET_ORDER)) { + if (!preg_match_all($regex, $stdout, $matches, PREG_SET_ORDER)) { // Output with no matches. This might be a configuration error, but more // likely it's something like "No lint errors." and the user just hasn't // written a sufficiently powerful/ridiculous regexp to capture it into an @@ -228,7 +268,7 @@ "ArcanistScriptAndRegexLinter: ". "configuration captured a 'throw' named capturing group, ". "'{$throw}'. Script output:\n". - $output); + $stdout); } if (!empty($match['halt'])) { @@ -272,59 +312,14 @@ } -/* -( Linter Information )------------------------------------------------- */ - - /** - * Return the short name of the linter. - * - * @return string Short linter identifier. - * - * @task linterinfo - */ - public function getLinterName() { - return 'S&RX'; - } - - public function getLinterConfigurationName() { - return 'script-and-regex'; - } - - public function getLinterConfigurationOptions() { - // These fields are optional only to avoid breaking things. - $options = array( - 'script' => array( - 'type' => 'optional string', - 'help' => pht('Script to execute.'), - ), - 'regex' => array( - 'type' => 'optional regex', - 'help' => pht('The regex to process output with.'), - ), - ); - - return $options + parent::getLinterConfigurationOptions(); - } - - public function setLinterConfigurationValue($key, $value) { - switch ($key) { - case 'script': - $this->script = $value; - return; - case 'regex': - $this->regex = $value; - return; - } - - return parent::setLinterConfigurationValue($key, $value); - } - /* -( Parsing Output )----------------------------------------------------- */ /** * Get the line and character of the message from the regex match. * - * @param dict Captured groups from regex. - * @return pair Line and character of the message. + * @param dict Captured groups from regex. + * @param string The path of the current file being linted. + * @return pair Line and character of the message. * * @task parse */ @@ -344,22 +339,16 @@ /** * Map the regex matching groups to a message severity. We look for either - * a nonempty severity name group like 'error', or a group called 'severity' + * a nonempty severity name group like `error`, or a group called `severity` * with a valid name. * - * @param dict Captured groups from regex. + * @param dict Captured groups from regex. * @return const @{class:ArcanistLintSeverity} constant. * * @task parse */ private function getMatchSeverity(array $match) { - $map = array( - 'error' => ArcanistLintSeverity::SEVERITY_ERROR, - 'warning' => ArcanistLintSeverity::SEVERITY_WARNING, - 'autofix' => ArcanistLintSeverity::SEVERITY_AUTOFIX, - 'advice' => ArcanistLintSeverity::SEVERITY_ADVICE, - 'disabled' => ArcanistLintSeverity::SEVERITY_DISABLED, - ); + $map = ArcanistLintSeverity::getSeverityMap(); $severity_name = strtolower(idx($match, 'severity')); @@ -378,14 +367,14 @@ /* -( Validating Configuration )------------------------------------------- */ /** - * Load, validate, and return the "script" configuration. + * Load, validate, and return the configured script. * - * @return string The shell command fragment to use to run the linter. + * @return string The shell command fragment to use to run the linter. * * @task config */ private function getConfiguredScript() { - if (strlen($this->script)) { + if ($this->script) { return $this->script; } @@ -403,14 +392,14 @@ } /** - * Load, validate, and return the "regex" configuration. + * Load, validate, and return the configured regex. * - * @return string A valid PHP PCRE regular expression. + * @return string A valid PHP PCRE regular expression. * * @task config */ private function getConfiguredRegex() { - if (strlen($this->regex)) { + if ($this->regex) { return $this->regex; } @@ -418,7 +407,10 @@ $config = $this->getDeprecatedConfiguration($key); if (!$config) { - throw new ArcanistUsageException('Parameter missing: regex'); + throw new ArcanistUsageException( + pht( + 'Linter is missing a required parameter: %s.', + 'regex')); } // NOTE: preg_match() returns 0 for no matches and false for compile error; @@ -427,9 +419,7 @@ $ok = preg_match($config, 'syntax-check'); if ($ok === false) { throw new ArcanistUsageException( - "ArcanistScriptAndRegexLinter: ". - "Regex '{$config}' does not compile. You must configure '{$key}' with ". - "a valid PHP PCRE regex, including delimiters."); + pht("Regex '%s' does not compile.", $config)); } return $config; diff --git a/src/lint/linter/__tests__/ArcanistScriptAndRegexLinterTestCase.php b/src/lint/linter/__tests__/ArcanistScriptAndRegexLinterTestCase.php new file mode 100644 --- /dev/null +++ b/src/lint/linter/__tests__/ArcanistScriptAndRegexLinterTestCase.php @@ -0,0 +1,10 @@ +executeTestsInDirectory(dirname(__FILE__).'/script-and-regex/'); + } + +} diff --git a/src/lint/linter/__tests__/script-and-regex/ack-grep.lint-test b/src/lint/linter/__tests__/script-and-regex/ack-grep.lint-test new file mode 100644 --- /dev/null +++ b/src/lint/linter/__tests__/script-and-regex/ack-grep.lint-test @@ -0,0 +1,10 @@ +test +~~~~~~~~~~ +error:1:1 +~~~~~~~~~~ +{ + "config": { + "script-and-regex.script": "ack-grep --noenv --with-filename --column --nocolor 'test'" + "script-and-regex.regex": "(^(?P\d+):(?P\d+):(?P.*)$)" + } +}