From be67df611856ceb31b31bb45f39be1659702b655 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 31 Aug 2017 09:14:07 -0700 Subject: [PATCH] Extract and cover the logic for "trimming" a lint message Summary: Ref T9846. Sometimes, a lint message says to replace "the big bad wolf" with "the huge bad wolf": that is, the original and replacement text are the same at the beginning, or the end, or both. To make this easier for humans to understand, we want to just show that "big" is being replaced with "huge", not that the entire phrase is being replaced. This logic currently happens inline in console rendering. Pull it out and cover it so a future change can simplify console rendering. Test Plan: Ran unit tests. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9846 Differential Revision: https://secure.phabricator.com/D18509 --- src/__phutil_library_map__.php | 2 + src/lint/ArcanistLintMessage.php | 71 ++++++++++++++++ .../__tests__/ArcanistLintMessageTestCase.php | 80 +++++++++++++++++++ src/lint/linter/__tests__/xml/attr2.lint-test | 4 - .../ArcanistConsoleLintRendererTestCase.php | 9 +++ .../renderer/__tests__/data/overlap.expect | 8 ++ src/lint/renderer/__tests__/data/overlap.txt | 1 + 7 files changed, 171 insertions(+), 4 deletions(-) create mode 100644 src/lint/__tests__/ArcanistLintMessageTestCase.php delete mode 100644 src/lint/linter/__tests__/xml/attr2.lint-test create mode 100644 src/lint/renderer/__tests__/data/overlap.expect create mode 100644 src/lint/renderer/__tests__/data/overlap.txt diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 00590037..7ab5a448 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -222,6 +222,7 @@ phutil_register_library_map(array( 'ArcanistLibraryTestCase' => '__tests__/ArcanistLibraryTestCase.php', 'ArcanistLintEngine' => 'lint/engine/ArcanistLintEngine.php', 'ArcanistLintMessage' => 'lint/ArcanistLintMessage.php', + 'ArcanistLintMessageTestCase' => 'lint/__tests__/ArcanistLintMessageTestCase.php', 'ArcanistLintPatcher' => 'lint/ArcanistLintPatcher.php', 'ArcanistLintRenderer' => 'lint/renderer/ArcanistLintRenderer.php', 'ArcanistLintResult' => 'lint/ArcanistLintResult.php', @@ -638,6 +639,7 @@ phutil_register_library_map(array( 'ArcanistLibraryTestCase' => 'PhutilLibraryTestCase', 'ArcanistLintEngine' => 'Phobject', 'ArcanistLintMessage' => 'Phobject', + 'ArcanistLintMessageTestCase' => 'PhutilTestCase', 'ArcanistLintPatcher' => 'Phobject', 'ArcanistLintRenderer' => 'Phobject', 'ArcanistLintResult' => 'Phobject', diff --git a/src/lint/ArcanistLintMessage.php b/src/lint/ArcanistLintMessage.php index dafc28ba..eb575f58 100644 --- a/src/lint/ArcanistLintMessage.php +++ b/src/lint/ArcanistLintMessage.php @@ -274,4 +274,75 @@ final class ArcanistLintMessage extends Phobject { return $value; } + public function newTrimmedMessage() { + if (!$this->isPatchable()) { + return clone $this; + } + + // If the original and replacement text have a similar prefix or suffix, + // we trim it to reduce the size of the diff we show to the user. + + $replacement = $this->getReplacementText(); + $original = $this->getOriginalText(); + + $replacement_length = strlen($replacement); + $original_length = strlen($original); + + $minimum_length = min($original_length, $replacement_length); + + $prefix_length = 0; + for ($ii = 0; $ii < $minimum_length; $ii++) { + if ($original[$ii] !== $replacement[$ii]) { + break; + } + $prefix_length++; + } + + // NOTE: The two strings can't be the same because the message won't be + // "patchable" if they are, so we don't need a special check for the case + // where the entire string is a shared prefix. + + $suffix_length = 0; + for ($ii = 1; $ii <= $minimum_length; $ii++) { + $original_char = $original[$original_length - $ii]; + $replacement_char = $replacement[$replacement_length - $ii]; + if ($original_char !== $replacement_char) { + break; + } + $suffix_length++; + } + + if ($suffix_length) { + $original = substr($original, 0, -$suffix_length); + $replacement = substr($replacement, 0, -$suffix_length); + } + + $line = $this->getLine(); + $char = $this->getChar(); + + if ($prefix_length) { + $prefix = substr($original, 0, $prefix_length); + + $original = substr($original, $prefix_length); + $replacement = substr($replacement, $prefix_length); + + // If we've removed a prefix, we need to push the character and line + // number for the warning forward to account for the characters we threw + // away. + for ($ii = 0; $ii < $prefix_length; $ii++) { + $char++; + if ($prefix[$ii] == "\n") { + $line++; + $char = 1; + } + } + } + + return id(clone $this) + ->setOriginalText($original) + ->setReplacementText($replacement) + ->setLine($line) + ->setChar($char); + } + } diff --git a/src/lint/__tests__/ArcanistLintMessageTestCase.php b/src/lint/__tests__/ArcanistLintMessageTestCase.php new file mode 100644 index 00000000..ae02b2b6 --- /dev/null +++ b/src/lint/__tests__/ArcanistLintMessageTestCase.php @@ -0,0 +1,80 @@ + array( + 'old' => 'a', + 'new' => 'b', + 'old.expect' => 'a', + 'new.expect' => 'b', + 'line' => 1, + 'char' => 1, + ), + 'prefix' => array( + 'old' => 'ever after', + 'new' => 'evermore', + 'old.expect' => ' after', + 'new.expect' => 'more', + 'line' => 1, + 'char' => 5, + ), + 'suffix' => array( + 'old' => 'arcane archaeology', + 'new' => 'mythic archaeology', + 'old.expect' => 'arcane', + 'new.expect' => 'mythic', + 'line' => 1, + 'char' => 1, + ), + 'both' => array( + 'old' => 'large red apple', + 'new' => 'large blue apple', + 'old.expect' => 'red', + 'new.expect' => 'blue', + 'line' => 1, + 'char' => 7, + ), + 'prefix-newline' => array( + 'old' => "four score\nand five years ago", + 'new' => "four score\nand seven years ago", + 'old.expect' => 'five', + 'new.expect' => 'seven', + 'line' => 2, + 'char' => 5, + ), + ); + + foreach ($map as $key => $test_case) { + $message = id(new ArcanistLintMessage()) + ->setOriginalText($test_case['old']) + ->setReplacementText($test_case['new']) + ->setLine(1) + ->setChar(1); + + $actual = $message->newTrimmedMessage(); + + $this->assertEqual( + $test_case['old.expect'], + $actual->getOriginalText(), + pht('Original text for "%s".', $key)); + + $this->assertEqual( + $test_case['new.expect'], + $actual->getReplacementText(), + pht('Replacement text for "%s".', $key)); + + $this->assertEqual( + $test_case['line'], + $actual->getLine(), + pht('Line for "%s".', $key)); + + $this->assertEqual( + $test_case['char'], + $actual->getChar(), + pht('Char for "%s".', $key)); + } + } +} diff --git a/src/lint/linter/__tests__/xml/attr2.lint-test b/src/lint/linter/__tests__/xml/attr2.lint-test deleted file mode 100644 index 021e0200..00000000 --- a/src/lint/linter/__tests__/xml/attr2.lint-test +++ /dev/null @@ -1,4 +0,0 @@ -