From 0c11b5c70c60c9cdd7ad031d33c50420279b3679 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Aug 2011 13:07:01 -0700 Subject: [PATCH] Allow XHPAST lint name rules to be overridden through configuration Summary: See T326. Allow lint rules to be selectively overridden, e.g. for Conduit methods. Since FB has a long history of suggesting crazy patches for this stuff I think we're safer just adding a hook class than trying to do some kind of regexp magic. Test Plan: Wrote a hook for Phabricator and linted some Conduit files without issues. Ran unit tests. Reviewers: nh, jungejason, tuomaspelkonen, aran Reviewed By: nh CC: aran, nh Differential Revision: 874 --- src/__phutil_library_map__.php | 1 + .../linter/xhpast/ArcanistXHPASTLinter.php | 160 ++++++++++++------ .../ArcanistXHPASTLintNamingHook.php | 60 +++++++ .../linter/xhpast/naminghook/__init__.php | 10 ++ 4 files changed, 176 insertions(+), 55 deletions(-) create mode 100644 src/lint/linter/xhpast/naminghook/ArcanistXHPASTLintNamingHook.php create mode 100644 src/lint/linter/xhpast/naminghook/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b5e28bac..7d9ac707 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -80,6 +80,7 @@ phutil_register_library_map(array( 'ArcanistUsageException' => 'exception/usage', 'ArcanistUserAbortException' => 'exception/usage/userabort', 'ArcanistWorkingCopyIdentity' => 'workingcopyidentity', + 'ArcanistXHPASTLintNamingHook' => 'lint/linter/xhpast/naminghook', 'ArcanistXHPASTLinter' => 'lint/linter/xhpast', 'ArcanistXHPASTLinterTestCase' => 'lint/linter/xhpast/__tests__', 'BranchInfo' => 'branch', diff --git a/src/lint/linter/xhpast/ArcanistXHPASTLinter.php b/src/lint/linter/xhpast/ArcanistXHPASTLinter.php index cf616f4c..09af5d15 100644 --- a/src/lint/linter/xhpast/ArcanistXHPASTLinter.php +++ b/src/lint/linter/xhpast/ArcanistXHPASTLinter.php @@ -665,27 +665,37 @@ class ArcanistXHPASTLinter extends ArcanistLinter { } protected function lintNamingConventions($root) { + + // We're going to build up a list of tuples + // and then try to instantiate a hook class which has the opportunity to + // override us. + $names = array(); + $classes = $root->selectDescendantsOfType('n_CLASS_DECLARATION'); foreach ($classes as $class) { $name_token = $class->getChildByIndex(1); $name_string = $name_token->getConcreteString(); $is_xhp = ($name_string[0] == ':'); if ($is_xhp) { - if (!$this->isLowerCaseWithXHP($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: xhp elements should be named using '. - 'lower case.'); - } + $names[] = array( + 'xhp-class', + $name_string, + $name_token, + $this->isLowerCaseWithXHP($name_string) + ? null + : 'Follow naming conventions: XHP elements should be named using '. + 'lower case.', + ); } else { - if (!$this->isUpperCamelCase($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: classes should be named using '. - 'UpperCamelCase.'); - } + $names[] = array( + 'class', + $name_string, + $name_token, + $this->isUpperCamelCase($name_string) + ? null + : 'Follow naming conventions: classes should be named using '. + 'UpperCamelCase.', + ); } } @@ -693,13 +703,15 @@ class ArcanistXHPASTLinter extends ArcanistLinter { foreach ($ifaces as $iface) { $name_token = $iface->getChildByIndex(1); $name_string = $name_token->getConcreteString(); - if (!$this->isUpperCamelCase($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: interfaces should be named using '. - 'UpperCamelCase.'); - } + $names[] = array( + 'interface', + $name_string, + $name_token, + $this->isUpperCamelCase($name_string) + ? null + : 'Follow naming conventions: interfaces should be named using '. + 'UpperCamelCase.', + ); } @@ -711,13 +723,15 @@ class ArcanistXHPASTLinter extends ArcanistLinter { continue; } $name_string = $name_token->getConcreteString(); - if (!$this->isLowercaseWithUnderscores($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: functions should be named using '. - 'lowercase_with_underscores.'); - } + $names[] = array( + 'function', + $name_string, + $name_token, + $this->isLowercaseWithUnderscores($name_string) + ? null + : 'Follow naming conventions: functions should be named using '. + 'lowercase_with_underscores.', + ); } @@ -725,13 +739,15 @@ class ArcanistXHPASTLinter extends ArcanistLinter { foreach ($methods as $method) { $name_token = $method->getChildByIndex(2); $name_string = $name_token->getConcreteString(); - if (!$this->isLowerCamelCase($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: methods should be named using '. - 'lowerCamelCase.'); - } + $names[] = array( + 'method', + $name_string, + $name_token, + $this->isLowerCamelCase($name_string) + ? null + : 'Follow naming conventions: methods should be named using '. + 'lowerCamelCase.', + ); } @@ -740,13 +756,15 @@ class ArcanistXHPASTLinter extends ArcanistLinter { foreach ($param_list->getChildren() as $param) { $name_token = $param->getChildByIndex(1); $name_string = $name_token->getConcreteString(); - if (!$this->isLowercaseWithUnderscores($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: parameters should be named using '. - 'lowercase_with_underscores.'); - } + $names[] = array( + 'parameter', + $name_string, + $name_token, + $this->isLowercaseWithUnderscores($name_string) + ? null + : 'Follow naming conventions: parameters should be named using '. + 'lowercase_with_underscores.', + ); } } @@ -757,13 +775,15 @@ class ArcanistXHPASTLinter extends ArcanistLinter { foreach ($constant_list->getChildren() as $constant) { $name_token = $constant->getChildByIndex(0); $name_string = $name_token->getConcreteString(); - if (!$this->isUppercaseWithUnderscores($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: class constants should be named using '. - 'UPPERCASE_WITH_UNDERSCORES.'); - } + $names[] = array( + 'constant', + $name_string, + $name_token, + $this->isUppercaseWithUnderscores($name_string) + ? null + : 'Follow naming conventions: class constants should be named '. + 'using UPPERCASE_WITH_UNDERSCORES.', + ); } } @@ -775,15 +795,45 @@ class ArcanistXHPASTLinter extends ArcanistLinter { } $name_token = $prop->getChildByIndex(0); $name_string = $name_token->getConcreteString(); - if (!$this->isLowerCamelCase($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: class properties should be named '. - 'using lowerCamelCase.'); + $names[] = array( + 'member', + $name_string, + $name_token, + $this->isLowerCamelCase($name_string) + ? null + : 'Follow naming conventions: class properties should be named '. + 'using lowerCamelCase.', + ); + } + } + + $engine = $this->getEngine(); + $working_copy = $engine->getWorkingCopy(); + + if ($working_copy) { + // If a naming hook is configured, give it a chance to override the + // default results for all the symbol names. + $hook_class = $working_copy->getConfig('lint.xhpast.naminghook'); + if ($hook_class) { + $hook_obj = newv($hook_class, array()); + foreach ($names as $k => $name_attrs) { + list($type, $name, $token, $default) = $name_attrs; + $result = $hook_obj->lintSymbolName($type, $name, $default); + $names[$k][3] = $result; } } } + + // Raise anything we're left with. + foreach ($names as $k => $name_attrs) { + list($type, $name, $token, $result) = $name_attrs; + if ($result) { + $this->raiseLintAtNode( + $token, + self::LINT_NAMING_CONVENTIONS, + $result); + } + } } protected function isUpperCamelCase($str) { diff --git a/src/lint/linter/xhpast/naminghook/ArcanistXHPASTLintNamingHook.php b/src/lint/linter/xhpast/naminghook/ArcanistXHPASTLintNamingHook.php new file mode 100644 index 00000000..e526f555 --- /dev/null +++ b/src/lint/linter/xhpast/naminghook/ArcanistXHPASTLintNamingHook.php @@ -0,0 +1,60 @@ + + } + + /** + * Callback invoked for each symbol, which can override the default + * determination of name validity or accept it by returning $default. The + * symbol types are: xhp-class, class, interface, function, method, parameter, + * constant, and member. + * + * For example, if you want to ban all symbols with "quack" in them and + * otherwise accept all the defaults, except allow any naming convention for + * methods with "duck" in them, you might implement the method like this: + * + * if (preg_match('/quack/i', $name)) { + * return 'Symbol names containing "quack" are forbidden.'; + * } + * if ($type == 'method' && preg_match('/duck/i', $name)) { + * return null; // Always accept. + * } + * return $default; + * + * @param string The symbol type. + * @param string The symbol name. + * @param string|null The default result from the main rule engine. + * @return string|null Null to accept the name, or a message to reject it + * with. You should return the default value if you don't + * want to specifically provide an override. + * @task override + */ + abstract public function lintSymbolName($type, $name, $default); + +} diff --git a/src/lint/linter/xhpast/naminghook/__init__.php b/src/lint/linter/xhpast/naminghook/__init__.php new file mode 100644 index 00000000..da02b025 --- /dev/null +++ b/src/lint/linter/xhpast/naminghook/__init__.php @@ -0,0 +1,10 @@ +