From 7870e7f2e46d220ef39dfdf1cc3f4322a92e5198 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 11 May 2014 18:39:28 -0700 Subject: [PATCH] Warn when accessing deprecated lint config Summary: Ref T2039. Addresses two issues: - Issues a warning for use of config which is deprecated by `.arclint`. - We no longer require an engine to be present for these linters, so `arc linters` doesn't fatal if they aren't configured. Test Plan: - Ran `arc linters` in a repo with no JSHint. - Ran `arc linters` in a repo with junk in .arcconfig and got a warning when it was read. Verified it still took effect. Reviewers: chad, btrahan, joshuaspence Reviewed By: joshuaspence Subscribers: epriestley Maniphest Tasks: T2039 Differential Revision: https://secure.phabricator.com/D9058 --- src/lint/engine/ComprehensiveLintEngine.php | 2 +- src/lint/linter/ArcanistCSSLintLinter.php | 8 +--- src/lint/linter/ArcanistCpplintLinter.php | 9 ++--- src/lint/linter/ArcanistExternalLinter.php | 2 +- src/lint/linter/ArcanistFlake8Linter.php | 15 +++---- src/lint/linter/ArcanistJSHintLinter.php | 10 ++--- src/lint/linter/ArcanistLinter.php | 45 +++++++++++++++++++++ src/lint/linter/ArcanistPEP8Linter.php | 13 ++---- src/lint/linter/ArcanistPhpcsLinter.php | 13 ++---- src/lint/linter/ArcanistPyFlakesLinter.php | 5 +-- src/lint/linter/ArcanistRubyLinter.php | 4 +- src/lint/linter/ArcanistScalaSBTLinter.php | 17 +------- 12 files changed, 72 insertions(+), 71 deletions(-) diff --git a/src/lint/engine/ComprehensiveLintEngine.php b/src/lint/engine/ComprehensiveLintEngine.php index 06555014..9d46cfb4 100644 --- a/src/lint/engine/ComprehensiveLintEngine.php +++ b/src/lint/engine/ComprehensiveLintEngine.php @@ -33,7 +33,7 @@ final class ComprehensiveLintEngine extends ArcanistLintEngine { $py_paths = preg_grep('/\.py$/', $paths); $linters[] = id(new ArcanistPyFlakesLinter())->setPaths($py_paths); $linters[] = id(new ArcanistPEP8Linter()) - ->setConfig(array('options' => $this->getPEP8WithTextOptions())) + ->setConfig(array('flags' => array($this->getPEP8WithTextOptions()))) ->setPaths($py_paths); $linters[] = id(new ArcanistRubyLinter()) diff --git a/src/lint/linter/ArcanistCSSLintLinter.php b/src/lint/linter/ArcanistCSSLintLinter.php index 0eb1f0b8..a488ed34 100644 --- a/src/lint/linter/ArcanistCSSLintLinter.php +++ b/src/lint/linter/ArcanistCSSLintLinter.php @@ -34,15 +34,11 @@ final class ArcanistCSSLintLinter extends ArcanistExternalLinter { } public function getDefaultFlags() { - // TODO: Deprecation warning. - $config = $this->getEngine()->getConfigurationManager(); - return $config->getConfigFromAnySource('lint.csslint.options', array()); + return $this->getDeprecatedConfiguration('lint.csslint.options', array()); } public function getDefaultBinary() { - // TODO: Deprecation warning. - $config = $this->getEngine()->getConfigurationManager(); - return $config->getConfigFromAnySource('lint.csslint.bin', 'csslint'); + return $this->getDeprecatedConfiguration('lint.csslint.bin', 'csslint'); } public function getVersion() { diff --git a/src/lint/linter/ArcanistCpplintLinter.php b/src/lint/linter/ArcanistCpplintLinter.php index 41974eed..3f660066 100644 --- a/src/lint/linter/ArcanistCpplintLinter.php +++ b/src/lint/linter/ArcanistCpplintLinter.php @@ -14,10 +14,8 @@ final class ArcanistCpplintLinter extends ArcanistExternalLinter { } public function getDefaultBinary() { - // TODO: Warn that all of this is deprecated. - $config = $this->getEngine()->getConfigurationManager(); - $prefix = $config->getConfigFromAnySource('lint.cpplint.prefix'); - $bin = $config->getConfigFromAnySource('lint.cpplint.bin', 'cpplint.py'); + $prefix = $this->getDeprecatedConfiguration('lint.cpplint.prefix'); + $bin = $this->getDeprecatedConfiguration('lint.cpplint.bin', 'cpplint.py'); if ($prefix) { return $prefix.'/'.$bin; @@ -44,8 +42,7 @@ final class ArcanistCpplintLinter extends ArcanistExternalLinter { } protected function getDefaultFlags() { - $config = $this->getEngine()->getConfigurationManager(); - return $config->getConfigFromAnySource('lint.cpplint.options', array()); + return $this->getDeprecatedConfiguration('lint.cpplint.options', array()); } protected function parseLinterOutput($path, $err, $stdout, $stderr) { diff --git a/src/lint/linter/ArcanistExternalLinter.php b/src/lint/linter/ArcanistExternalLinter.php index 70612d5a..d05fa219 100644 --- a/src/lint/linter/ArcanistExternalLinter.php +++ b/src/lint/linter/ArcanistExternalLinter.php @@ -150,7 +150,7 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { * @task bin */ final public function setFlags($flags) { - $this->flags = (array) $flags; + $this->flags = (array)$flags; return $this; } diff --git a/src/lint/linter/ArcanistFlake8Linter.php b/src/lint/linter/ArcanistFlake8Linter.php index cfac19c7..ef810efb 100644 --- a/src/lint/linter/ArcanistFlake8Linter.php +++ b/src/lint/linter/ArcanistFlake8Linter.php @@ -29,21 +29,18 @@ final class ArcanistFlake8Linter extends ArcanistExternalLinter { } public function getDefaultFlags() { - // TODO: Deprecated. - $config = $this->getEngine()->getConfigurationManager(); - return $config->getConfigFromAnySource('lint.flake8.options', array()); + return $this->getDeprecatedConfiguration('lint.flake8.options', array()); } public function getDefaultBinary() { - $config = $this->getEngine()->getConfigurationManager(); - $prefix = $config->getConfigFromAnySource('lint.flake8.prefix'); - $bin = $config->getConfigFromAnySource('lint.flake8.bin', 'flake8'); + $prefix = $this->getDeprecatedConfiguration('lint.flake8.prefix'); + $bin = $this->getDeprecatedConfiguration('lint.flake8.bin', 'flake8'); - if ($prefix || ($bin != 'flake8')) { + if ($prefix) { return $prefix.'/'.$bin; + } else { + return $bin; } - - return 'flake8'; } public function getVersion() { diff --git a/src/lint/linter/ArcanistJSHintLinter.php b/src/lint/linter/ArcanistJSHintLinter.php index ef6ec9db..d05bcecc 100644 --- a/src/lint/linter/ArcanistJSHintLinter.php +++ b/src/lint/linter/ArcanistJSHintLinter.php @@ -35,9 +35,8 @@ final class ArcanistJSHintLinter extends ArcanistExternalLinter { } public function getDefaultBinary() { - $config = $this->getEngine()->getConfigurationManager(); - $prefix = $config->getConfigFromAnySource('lint.jshint.prefix'); - $bin = $config->getConfigFromAnySource('lint.jshint.bin', 'jshint'); + $prefix = $this->getDeprecatedConfiguration('lint.jshint.prefix'); + $bin = $this->getDeprecatedConfiguration('lint.jshint.bin', 'jshint'); if ($prefix) { return $prefix.'/'.$bin; @@ -81,12 +80,11 @@ final class ArcanistJSHintLinter extends ArcanistExternalLinter { } protected function getDefaultFlags() { - $config_manager = $this->getEngine()->getConfigurationManager(); - $options = $config_manager->getConfigFromAnySource( + $options = $this->getDeprecatedConfiguration( 'lint.jshint.options', array()); - $config = $config_manager->getConfigFromAnySource('lint.jshint.config'); + $config = $this->getDeprecatedConfiguration('lint.jshint.config'); if ($config) { $options[] = '--config='.$config; } diff --git a/src/lint/linter/ArcanistLinter.php b/src/lint/linter/ArcanistLinter.php index caae477d..e8847512 100644 --- a/src/lint/linter/ArcanistLinter.php +++ b/src/lint/linter/ArcanistLinter.php @@ -438,4 +438,49 @@ abstract class ArcanistLinter { return $code; } + + /** + * Retrieve an old lint configuration value from `.arcconfig` or a similar + * source. + * + * Modern linters should use @{method:getConfig} to read configuration from + * `.arclint`. + * + * @param string Configuration key to retrieve. + * @param wild Default value to return if key is not present in config. + * @return wild Configured value, or default if no configuration exists. + */ + protected function getDeprecatedConfiguration($key, $default = null) { + + // If we're being called in a context without an engine (probably from + // `arc linters`), just return the default value. + if (!$this->engine) { + return $default; + } + + $config = $this->getEngine()->getConfigurationManager(); + + // Construct a sentinel object so we can tell if we're reading config + // or not. + $sentinel = (object)array(); + $result = $config->getConfigFromAnySource($key, $sentinel); + + // If we read config, warn the user that this mechanism is deprecated and + // discouraged. + if ($result !== $sentinel) { + $console = PhutilConsole::getConsole(); + $console->writeErr( + "**%s**: %s\n", + pht('Deprecation Warning'), + pht( + 'Configuration option "%s" is deprecated. Generally, linters should '. + 'now be configured using an `.arclint` file. See "Arcanist User '. + 'Guide: Lint" in the documentation for more information.', + $key)); + return $result; + } + + return $default; + } + } diff --git a/src/lint/linter/ArcanistPEP8Linter.php b/src/lint/linter/ArcanistPEP8Linter.php index 463bb44d..e02134d1 100644 --- a/src/lint/linter/ArcanistPEP8Linter.php +++ b/src/lint/linter/ArcanistPEP8Linter.php @@ -28,11 +28,7 @@ final class ArcanistPEP8Linter extends ArcanistExternalLinter { } public function getDefaultFlags() { - // TODO: Warn that all of this is deprecated. - $config = $this->getEngine()->getConfigurationManager(); - return $config->getConfigFromAnySource( - 'lint.pep8.options', - $this->getConfig('options', array())); + return $this->getDeprecatedConfiguration('lint.pep8.options', array()); } public function shouldUseInterpreter() { @@ -48,12 +44,9 @@ final class ArcanistPEP8Linter extends ArcanistExternalLinter { return 'pep8'; } - $config = $this->getEngine()->getConfigurationManager(); - $old_prefix = $config->getConfigFromAnySource('lint.pep8.prefix'); - $old_bin = $config->getConfigFromAnySource('lint.pep8.bin'); - + $old_prefix = $this->getDeprecatedConfiguration('lint.pep8.prefix'); + $old_bin = $this->getDeprecatedConfiguration('lint.pep8.bin'); if ($old_prefix || $old_bin) { - // TODO: Deprecation warning. $old_bin = nonempty($old_bin, 'pep8'); return $old_prefix.'/'.$old_bin; } diff --git a/src/lint/linter/ArcanistPhpcsLinter.php b/src/lint/linter/ArcanistPhpcsLinter.php index 92631251..1dbb10a7 100644 --- a/src/lint/linter/ArcanistPhpcsLinter.php +++ b/src/lint/linter/ArcanistPhpcsLinter.php @@ -48,13 +48,8 @@ final class ArcanistPhpcsLinter extends ArcanistExternalLinter { } public function getDefaultFlags() { - // TODO: Deprecation warnings. - - $config = $this->getEngine()->getConfigurationManager(); - - $options = $config->getConfigFromAnySource('lint.phpcs.options', array()); - - $standard = $config->getConfigFromAnySource('lint.phpcs.standard'); + $options = $this->getDeprecatedConfiguration('lint.phpcs.options', array()); + $standard = $this->getDeprecatedConfiguration('lint.phpcs.standard'); if (!empty($standard)) { if (is_array($options)) { $options[] = '--standard='.$standard; @@ -67,9 +62,7 @@ final class ArcanistPhpcsLinter extends ArcanistExternalLinter { } public function getDefaultBinary() { - // TODO: Deprecation warnings. - $config = $this->getEngine()->getConfigurationManager(); - return $config->getConfigFromAnySource('lint.phpcs.bin', 'phpcs'); + return $this->getDeprecatedConfiguration('lint.phpcs.bin', 'phpcs'); } public function getVersion() { diff --git a/src/lint/linter/ArcanistPyFlakesLinter.php b/src/lint/linter/ArcanistPyFlakesLinter.php index 7617bf16..19740a1e 100644 --- a/src/lint/linter/ArcanistPyFlakesLinter.php +++ b/src/lint/linter/ArcanistPyFlakesLinter.php @@ -28,9 +28,8 @@ final class ArcanistPyFlakesLinter extends ArcanistExternalLinter { } public function getDefaultBinary() { - $config = $this->getEngine()->getConfigurationManager(); - $prefix = $config->getConfigFromAnySource('lint.pyflakes.prefix'); - $bin = $config->getConfigFromAnySource('lint.pyflakes.bin', 'pyflakes'); + $prefix = $this->getDeprecatedConfiguration('lint.pyflakes.prefix'); + $bin = $this->getDeprecatedConfiguration('lint.pyflakes.bin', 'pyflakes'); if ($prefix) { return $prefix.'/'.$bin; diff --git a/src/lint/linter/ArcanistRubyLinter.php b/src/lint/linter/ArcanistRubyLinter.php index 739585d5..38c60702 100644 --- a/src/lint/linter/ArcanistRubyLinter.php +++ b/src/lint/linter/ArcanistRubyLinter.php @@ -26,9 +26,7 @@ final class ArcanistRubyLinter extends ArcanistExternalLinter { } public function getDefaultBinary() { - // TODO: Deprecation warning. - $config = $this->getEngine()->getConfigurationManager(); - $prefix = $config->getConfigFromAnySource('lint.ruby.prefix'); + $prefix = $this->getDeprecatedConfiguration('lint.ruby.prefix'); if ($prefix !== null) { $ruby_bin = $prefix.'ruby'; } diff --git a/src/lint/linter/ArcanistScalaSBTLinter.php b/src/lint/linter/ArcanistScalaSBTLinter.php index 4963373a..5e24f358 100644 --- a/src/lint/linter/ArcanistScalaSBTLinter.php +++ b/src/lint/linter/ArcanistScalaSBTLinter.php @@ -2,8 +2,6 @@ /** * Uses `sbt compile` to detect various warnings/errors in Scala code. - * - * @group linter */ final class ArcanistScalaSBTLinter extends ArcanistLinter { @@ -26,24 +24,11 @@ final class ArcanistScalaSBTLinter extends ArcanistLinter { private function getSBTPath() { $sbt_bin = "sbt"; - // Use the SBT prefix specified in the config file - $config = $this->getEngine()->getConfigurationManager(); - $prefix = $config->getConfigFromAnySource('lint.scala_sbt.prefix'); + $prefix = $this->getDeprecatedConfiguration('lint.scala_sbt.prefix'); if ($prefix !== null) { $sbt_bin = $prefix . $sbt_bin; } - if (!Filesystem::pathExists($sbt_bin)) { - - list($err) = exec_manual('which %s', $sbt_bin); - if ($err) { - throw new ArcanistUsageException( - "SBT does not appear to be installed on this system. Install it or ". - "add 'lint.scala_sbt.prefix' in your .arcconfig to point to ". - "the directory where it resides."); - } - } - return $sbt_bin; }