From 30ecf46c1167335f4fe81482628e4b10368a22b7 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Wed, 23 Apr 2014 16:22:49 -0700 Subject: [PATCH] Allow `ArcanistExternalLinter` flags to be specified as an array. Summary: Personally, I prefer to specify command lines flags as an array rather than a string. Test Plan: `arc unit` Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: aran, epriestley, Korvin, chad Differential Revision: https://secure.phabricator.com/D8387 --- src/lint/linter/ArcanistCSSLintLinter.php | 4 +- src/lint/linter/ArcanistExternalLinter.php | 45 ++++++++++++++-------- src/lint/linter/ArcanistFlake8Linter.php | 2 +- src/lint/linter/ArcanistPEP8Linter.php | 4 +- src/lint/linter/ArcanistPhpcsLinter.php | 12 ++++-- src/lint/linter/ArcanistRubyLinter.php | 2 +- 6 files changed, 45 insertions(+), 24 deletions(-) diff --git a/src/lint/linter/ArcanistCSSLintLinter.php b/src/lint/linter/ArcanistCSSLintLinter.php index 1f07a4b9..a69929cf 100644 --- a/src/lint/linter/ArcanistCSSLintLinter.php +++ b/src/lint/linter/ArcanistCSSLintLinter.php @@ -19,13 +19,13 @@ final class ArcanistCSSLintLinter extends ArcanistExternalLinter { } public function getMandatoryFlags() { - return '--format=lint-xml'; + return array('--format=lint-xml'); } public function getDefaultFlags() { // TODO: Deprecation warning. $config = $this->getEngine()->getConfigurationManager(); - return $config->getConfigFromAnySource('lint.csslint.options'); + return $config->getConfigFromAnySource('lint.csslint.options', array()); } public function getDefaultBinary() { diff --git a/src/lint/linter/ArcanistExternalLinter.php b/src/lint/linter/ArcanistExternalLinter.php index d8fde402..4a952359 100644 --- a/src/lint/linter/ArcanistExternalLinter.php +++ b/src/lint/linter/ArcanistExternalLinter.php @@ -117,11 +117,11 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { * Flags which are not mandatory should be provided in * @{method:getDefaultFlags} instead. * - * @return string|null Mandatory flags, like `"--format=xml"`. + * @return list Mandatory flags, like `"--format=xml"`. * @task bin */ protected function getMandatoryFlags() { - return null; + return array(); } @@ -133,11 +133,11 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { * * Default flags can be overridden with @{method:setFlags}. * - * @return string|null Overridable default flags. + * @return list Overridable default flags. * @task bin */ protected function getDefaultFlags() { - return null; + return array(); } @@ -145,12 +145,12 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { * Override default flags with custom flags. If not overridden, flags provided * by @{method:getDefaultFlags} are used. * - * @param string New flags. + * @param list New flags. * @return this * @task bin */ final public function setFlags($flags) { - $this->flags = $flags; + $this->flags = (array) $flags; return $this; } @@ -348,21 +348,32 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { * Get the composed flags for the executable, including both mandatory and * configured flags. * - * @return string Composed flags. + * @return list Composed flags. * @task exec */ protected function getCommandFlags() { - return csprintf( - '%C %C', - $this->getMandatoryFlags(), - coalesce($this->flags, $this->getDefaultFlags())); + $mandatory_flags = $this->getMandatoryFlags(); + if (!is_array($mandatory_flags)) { + phutil_deprecated( + 'String support for flags.', 'You should use list instead.'); + $mandatory_flags = (array) $mandatory_flags; + } + + $flags = nonempty($this->flags, $this->getDefaultFlags()); + if (!is_array($flags)) { + phutil_deprecated( + 'String support for flags.', 'You should use list instead.'); + $flags = (array) $flags; + } + + return array_merge($mandatory_flags, $flags); } protected function buildFutures(array $paths) { $executable = $this->getExecutableCommand(); - $bin = csprintf('%C %C', $executable, $this->getCommandFlags()); + $bin = csprintf('%C %Ls', $executable, $this->getCommandFlags()); $futures = array(); foreach ($paths as $path) { @@ -410,7 +421,7 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { public function getLinterConfigurationOptions() { $options = array( 'bin' => 'optional string | list', - 'flags' => 'optional string', + 'flags' => 'optional list', ); if ($this->shouldUseInterpreter()) { @@ -465,9 +476,13 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { throw new Exception( pht('None of the configured binaries can be located.')); case 'flags': - if (strlen($value)) { - $this->setFlags($value); + if (!is_array($value)) { + phutil_deprecated( + 'String support for flags.', + 'You should use list instead.'); + $value = (array) $value; } + $this->setFlags($value); return; } diff --git a/src/lint/linter/ArcanistFlake8Linter.php b/src/lint/linter/ArcanistFlake8Linter.php index 01ecd49c..36ecbd70 100644 --- a/src/lint/linter/ArcanistFlake8Linter.php +++ b/src/lint/linter/ArcanistFlake8Linter.php @@ -19,7 +19,7 @@ final class ArcanistFlake8Linter extends ArcanistExternalLinter { public function getDefaultFlags() { // TODO: Deprecated. $config = $this->getEngine()->getConfigurationManager(); - return $config->getConfigFromAnySource('lint.flake8.options', ''); + return $config->getConfigFromAnySource('lint.flake8.options', array()); } public function getDefaultBinary() { diff --git a/src/lint/linter/ArcanistPEP8Linter.php b/src/lint/linter/ArcanistPEP8Linter.php index 2f67a859..1058c8f1 100644 --- a/src/lint/linter/ArcanistPEP8Linter.php +++ b/src/lint/linter/ArcanistPEP8Linter.php @@ -17,7 +17,7 @@ final class ArcanistPEP8Linter extends ArcanistExternalLinter { public function getCacheVersion() { list($stdout) = execx('%C --version', $this->getExecutableCommand()); - return $stdout.$this->getCommandFlags(); + return $stdout.implode(' ', $this->getCommandFlags()); } public function getDefaultFlags() { @@ -25,7 +25,7 @@ final class ArcanistPEP8Linter extends ArcanistExternalLinter { $config = $this->getEngine()->getConfigurationManager(); return $config->getConfigFromAnySource( 'lint.pep8.options', - $this->getConfig('options')); + $this->getConfig('options', array())); } public function shouldUseInterpreter() { diff --git a/src/lint/linter/ArcanistPhpcsLinter.php b/src/lint/linter/ArcanistPhpcsLinter.php index efbb3fca..a5c292b2 100644 --- a/src/lint/linter/ArcanistPhpcsLinter.php +++ b/src/lint/linter/ArcanistPhpcsLinter.php @@ -26,7 +26,7 @@ final class ArcanistPhpcsLinter extends ArcanistExternalLinter { } public function getMandatoryFlags() { - return '--report=xml'; + return array('--report=xml'); } public function getInstallInstructions() { @@ -38,10 +38,16 @@ final class ArcanistPhpcsLinter extends ArcanistExternalLinter { $config = $this->getEngine()->getConfigurationManager(); - $options = $config->getConfigFromAnySource('lint.phpcs.options'); + $options = $config->getConfigFromAnySource('lint.phpcs.options', array()); $standard = $config->getConfigFromAnySource('lint.phpcs.standard'); - $options .= !empty($standard) ? ' --standard=' . $standard : ''; + if (!empty($standard)) { + if (is_array($options)) { + $options[] = '--standard='.$standard; + } else { + $options .= ' --standard='.$standard; + } + } return $options; } diff --git a/src/lint/linter/ArcanistRubyLinter.php b/src/lint/linter/ArcanistRubyLinter.php index 08619c79..2921f5f4 100644 --- a/src/lint/linter/ArcanistRubyLinter.php +++ b/src/lint/linter/ArcanistRubyLinter.php @@ -41,7 +41,7 @@ final class ArcanistRubyLinter extends ArcanistExternalLinter { protected function getMandatoryFlags() { // -w: turn on warnings // -c: check syntax - return '-w -c'; + return array('-w', '-c'); } protected function getDefaultMessageSeverity($code) {