From 7e61e43f6554a26fbf4f30ee0c05ec304ebb49ca Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 11 Feb 2019 14:21:31 -0800 Subject: [PATCH 1/3] Add version check whitelists for constants to the version compatibility lint rule Summary: Ref T13249. We currently allow `if (function_exists('X')) { X(); }` but not `if (defined('X')) { X; }`. Allow the latter. Test Plan: See D20145, which linted clean with this patch in place. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13249 Differential Revision: https://secure.phabricator.com/D20146 --- .../linter/__tests__/jshint/jshint.lint-test | 2 +- ...canistPHPCompatibilityXHPASTLinterRule.php | 39 +++++++++++++------ 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/lint/linter/__tests__/jshint/jshint.lint-test b/src/lint/linter/__tests__/jshint/jshint.lint-test index 37c13892..edb1c779 100644 --- a/src/lint/linter/__tests__/jshint/jshint.lint-test +++ b/src/lint/linter/__tests__/jshint/jshint.lint-test @@ -9,4 +9,4 @@ function f() { ~~~~~~~~~~ warning:3:8 error:7:1 -error:9: +error:9:1 diff --git a/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php index b2fa6770..f3dd662e 100644 --- a/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php @@ -26,6 +26,7 @@ final class ArcanistPHPCompatibilityXHPASTLinterRule $whitelist = array( 'class' => array(), 'function' => array(), + 'constant' => array(), ); $conditionals = $root->selectDescendantsOfType('n_IF'); @@ -51,6 +52,7 @@ final class ArcanistPHPCompatibilityXHPASTLinterRule case 'class_exists': case 'function_exists': case 'interface_exists': + case 'defined': $type = null; switch ($function_name) { case 'class_exists': @@ -64,6 +66,10 @@ final class ArcanistPHPCompatibilityXHPASTLinterRule case 'interface_exists': $type = 'interface'; break; + + case 'defined': + $type = 'constant'; + break; } $params = $function->getChildOfType(1, 'n_CALL_PARAMETER_LIST'); @@ -98,7 +104,6 @@ final class ArcanistPHPCompatibilityXHPASTLinterRule $min = idx($version, 'php.min'); $max = idx($version, 'php.max'); - // Check if whitelisted. $whitelisted = false; foreach (idx($whitelist['function'], $name, array()) as $range) { if (array_intersect($range, array_keys($node->getTokens()))) { @@ -178,18 +183,18 @@ final class ArcanistPHPCompatibilityXHPASTLinterRule $version = idx($compat_info['classes'], $name, $version); $min = idx($version, 'php.min'); $max = idx($version, 'php.max'); - // Check if whitelisted. - $whitelisted = false; - foreach (idx($whitelist['class'], $name, array()) as $range) { - if (array_intersect($range, array_keys($node->getTokens()))) { - $whitelisted = true; - break; - } - } - if ($whitelisted) { - continue; + $whitelisted = false; + foreach (idx($whitelist['class'], $name, array()) as $range) { + if (array_intersect($range, array_keys($node->getTokens()))) { + $whitelisted = true; + break; } + } + + if ($whitelisted) { + continue; + } if ($min && version_compare($min, $this->version, '>')) { $this->raiseLintAtNode( @@ -225,6 +230,18 @@ final class ArcanistPHPCompatibilityXHPASTLinterRule $min = idx($version, 'php.min'); $max = idx($version, 'php.max'); + $whitelisted = false; + foreach (idx($whitelist['constant'], $name, array()) as $range) { + if (array_intersect($range, array_keys($node->getTokens()))) { + $whitelisted = true; + break; + } + } + + if ($whitelisted) { + continue; + } + if ($min && version_compare($min, $this->version, '>')) { $this->raiseLintAtNode( $node, From 07a208d8fc4752ef2adc6f71c25001bb703fba53 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 14 Feb 2019 10:14:40 -0800 Subject: [PATCH 2/3] In "arc diff", warn when some reviewers are away even if not everyone is away Summary: Ref T13249. See PHI810. We currently warn you when //all// reviewers are away, but not when only some reviewers are away. This makes some amount of sense under the "anyone can accept anything" rules we sort of recommend, but a lot of installs realistically have tons of owner/package rules now. Instead, if any reviewers are away, show the user exactly who is away and until when, then make sure they don't want to make any adjustments. (We can do a better job of this after the toolsets change when we can use the new APIs, but this is an easy fix for now.) Test Plan: Created a revision with multiple reviewers, either some or all of whom were away. Got appropriate output and prompt behavior. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13249 Differential Revision: https://secure.phabricator.com/D20172 --- src/workflow/ArcanistDiffWorkflow.php | 55 +++++++++++++++++++++++---- 1 file changed, 47 insertions(+), 8 deletions(-) diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php index 86296ae5..873d5542 100644 --- a/src/workflow/ArcanistDiffWorkflow.php +++ b/src/workflow/ArcanistDiffWorkflow.php @@ -1820,17 +1820,56 @@ EOTEXT $this->checkRevisionOwnership(head($result)); break; case 'reviewers': - $untils = array(); + $away = array(); foreach ($result as $user) { - if (idx($user, 'currentStatus') == 'away') { - $untils[] = $user['currentStatusUntil']; + if (idx($user, 'currentStatus') != 'away') { + continue; } + + $username = $user['userName']; + $real_name = $user['realName']; + + if (strlen($real_name)) { + $name = pht('%s (%s)', $username, $real_name); + } else { + $name = pht('%s', $username); + } + + $away[] = array( + 'name' => $name, + 'until' => $user['currentStatusUntil'], + ); } - if (count($untils) == count($reviewers)) { - $until = date('l, M j Y', min($untils)); - $confirm = pht( - 'All reviewers are away until %s. Continue anyway?', - $until); + + if ($away) { + if (count($away) == count($reviewers)) { + $earliest_return = min(ipull($away, 'until')); + + $message = pht( + 'All reviewers are away until %s:', + date('l, M j Y', $earliest_return)); + } else { + $message = pht('Some reviewers are currently away:'); + } + + echo tsprintf( + "%s\n\n", + $message); + + $list = id(new PhutilConsoleList()); + foreach ($away as $spec) { + $list->addItem( + pht( + '%s (until %s)', + $spec['name'], + date('l, M j Y', $spec['until']))); + } + + echo tsprintf( + '%B', + $list->drawConsoleString()); + + $confirm = pht('Continue even though reviewers are unavailable?'); if (!phutil_console_confirm($confirm)) { throw new ArcanistUsageException( pht('Specify available reviewers and retry.')); From 9581dd0f52726b10c923038c28d5fe2170f0d1b6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 15 Feb 2019 09:53:23 -0800 Subject: [PATCH 3/3] Add Arcanist support for highlighting indent change intraline diffs Summary: Ref T13161. See D20181. This allows the intraline highlighter to accept new ">" and "<" spans and apply a different style for them. The input pattern is `list`. Each segment is `pair`, i.e. wrap the next `byte_length` bytes in a span of kind `kind`. Before this change, the possible kinds of segements are `0` (no intraline diff, do not highlight) or `1` (intraline diff, highlight in bright color). D20181 adds `<` (depth decreased) and `>` (depth increased). These are like `1`, but add a different class so the UI can handle them differently. Test Plan: See D20181. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13161 Differential Revision: https://secure.phabricator.com/D20182 --- src/difference/ArcanistDiffUtils.php | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/difference/ArcanistDiffUtils.php b/src/difference/ArcanistDiffUtils.php index a68a0532..0f798c9e 100644 --- a/src/difference/ArcanistDiffUtils.php +++ b/src/difference/ArcanistDiffUtils.php @@ -89,6 +89,9 @@ final class ArcanistDiffUtils extends Phobject { $highlight_o = ''; $highlight_c = ''; + $depth_in = ''; + $depth_out = ''; + $is_html = false; if ($str instanceof PhutilSafeHTML) { $is_html = true; @@ -107,11 +110,23 @@ final class ArcanistDiffUtils extends Phobject { $stack = array_shift($intra_stack); $s = $e; $e += $stack[1]; - } while ($stack[0] == 0); + } while ($stack[0] === 0); + + switch ($stack[0]) { + case '>': + $open_tag = $depth_in; + break; + case '<': + $open_tag = $depth_out; + break; + default: + $open_tag = $highlight_o; + break; + } } if (!$highlight && !$tag && !$ent && $p == $s) { - $buf .= $highlight_o; + $buf .= $open_tag; $highlight = true; } @@ -139,7 +154,7 @@ final class ArcanistDiffUtils extends Phobject { if ($tag && $str[$i] == '>') { $tag = false; if ($highlight) { - $buf .= $highlight_o; + $buf .= $open_tag; } }