From ab0d81bca292756bbfb1bf1cc42c9cae01fc7cc3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 31 Aug 2017 08:13:19 -0700 Subject: [PATCH 1/5] Add basic test coverage for lint console rendering Summary: Ref T9846. The algorithm here is fairly invovled, so lay down some test coverage before breaking it. Test Plan: Ran tests. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9846 Differential Revision: https://secure.phabricator.com/D18508 --- src/__phutil_library_map__.php | 2 + .../ArcanistConsoleLintRendererTestCase.php | 89 +++++++++++++++++++ .../renderer/__tests__/data/inline.expect | 8 ++ src/lint/renderer/__tests__/data/inline.txt | 1 + .../renderer/__tests__/data/simple.expect | 10 +++ src/lint/renderer/__tests__/data/simple.txt | 3 + 6 files changed, 113 insertions(+) create mode 100644 src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php create mode 100644 src/lint/renderer/__tests__/data/inline.expect create mode 100644 src/lint/renderer/__tests__/data/inline.txt create mode 100644 src/lint/renderer/__tests__/data/simple.expect create mode 100644 src/lint/renderer/__tests__/data/simple.txt diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2a3cfcf8..00590037 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -87,6 +87,7 @@ phutil_register_library_map(array( 'ArcanistConfigurationDrivenUnitTestEngine' => 'unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php', 'ArcanistConfigurationManager' => 'configuration/ArcanistConfigurationManager.php', 'ArcanistConsoleLintRenderer' => 'lint/renderer/ArcanistConsoleLintRenderer.php', + 'ArcanistConsoleLintRendererTestCase' => 'lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php', 'ArcanistConstructorParenthesesXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistConstructorParenthesesXHPASTLinterRule.php', 'ArcanistConstructorParenthesesXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistConstructorParenthesesXHPASTLinterRuleTestCase.php', 'ArcanistControlStatementSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistControlStatementSpacingXHPASTLinterRule.php', @@ -502,6 +503,7 @@ phutil_register_library_map(array( 'ArcanistConfigurationDrivenUnitTestEngine' => 'ArcanistUnitTestEngine', 'ArcanistConfigurationManager' => 'Phobject', 'ArcanistConsoleLintRenderer' => 'ArcanistLintRenderer', + 'ArcanistConsoleLintRendererTestCase' => 'PhutilTestCase', 'ArcanistConstructorParenthesesXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistConstructorParenthesesXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistControlStatementSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php new file mode 100644 index 00000000..aa07850e --- /dev/null +++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php @@ -0,0 +1,89 @@ + array( + 'line' => 1, + 'char' => 1, + 'original' => 'a', + 'replacement' => 'z', + ), + 'inline' => array( + 'line' => 1, + 'char' => 7, + 'original' => 'cat', + 'replacement' => 'dog', + ), + ); + + $defaults = array( + 'severity' => ArcanistLintSeverity::SEVERITY_WARNING, + 'name' => 'Lint Warning', + 'path' => 'path/to/example.c', + 'description' => 'Consider this.', + 'code' => 'WARN123', + ); + + foreach ($map as $key => $test_case) { + $data = $this->readTestData("{$key}.txt"); + $expect = $this->readTestData("{$key}.expect"); + + $test_case = $test_case + $defaults; + + $path = $test_case['path']; + $severity = $test_case['severity']; + $name = $test_case['name']; + $description = $test_case['description']; + $code = $test_case['code']; + + $line = $test_case['line']; + $char = $test_case['char']; + + $original = idx($test_case, 'original'); + $replacement = idx($test_case, 'replacement'); + + $message = id(new ArcanistLintMessage()) + ->setPath($path) + ->setSeverity($severity) + ->setName($name) + ->setDescription($description) + ->setCode($code) + ->setLine($line) + ->setChar($char) + ->setOriginalText($original) + ->setReplacementText($replacement); + + $result = id(new ArcanistLintResult()) + ->setPath($path) + ->setData($data) + ->addMessage($message); + + $renderer = new ArcanistConsoleLintRenderer(); + + try { + PhutilConsoleFormatter::disableANSI(true); + $actual = $renderer->renderLintResult($result); + PhutilConsoleFormatter::disableANSI(false); + } catch (Exception $ex) { + PhutilConsoleFormatter::disableANSI(false); + throw $ex; + } + + $this->assertEqual( + $expect, + $actual, + pht( + 'Lint rendering for "%s".', + $key)); + } + } + + private function readTestData($filename) { + $path = dirname(__FILE__).'/data/'.$filename; + return Filesystem::readFile($path); + } + +} diff --git a/src/lint/renderer/__tests__/data/inline.expect b/src/lint/renderer/__tests__/data/inline.expect new file mode 100644 index 00000000..eba4522b --- /dev/null +++ b/src/lint/renderer/__tests__/data/inline.expect @@ -0,0 +1,8 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + >>> - 1 adjudicated + + adjudidoged diff --git a/src/lint/renderer/__tests__/data/inline.txt b/src/lint/renderer/__tests__/data/inline.txt new file mode 100644 index 00000000..1907182f --- /dev/null +++ b/src/lint/renderer/__tests__/data/inline.txt @@ -0,0 +1 @@ +adjudicated diff --git a/src/lint/renderer/__tests__/data/simple.expect b/src/lint/renderer/__tests__/data/simple.expect new file mode 100644 index 00000000..43b9910f --- /dev/null +++ b/src/lint/renderer/__tests__/data/simple.expect @@ -0,0 +1,10 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + >>> - 1 a + + z + 2 b + 3 c diff --git a/src/lint/renderer/__tests__/data/simple.txt b/src/lint/renderer/__tests__/data/simple.txt new file mode 100644 index 00000000..de980441 --- /dev/null +++ b/src/lint/renderer/__tests__/data/simple.txt @@ -0,0 +1,3 @@ +a +b +c From be67df611856ceb31b31bb45f39be1659702b655 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 31 Aug 2017 09:14:07 -0700 Subject: [PATCH 2/5] 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 @@ -getMessages(); $path = $result->getPath(); + $data = $result->getData(); - $lines = explode("\n", $result->getData()); + $line_map = $this->newOffsetMap($data); $text = array(); - foreach ($messages as $message) { if (!$this->showAutofixPatches && $message->isAutofix()) { continue; @@ -57,7 +57,7 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { phutil_console_wrap($description, 4)); if ($message->hasFileContext()) { - $text[] = $this->renderContext($message, $lines); + $text[] = $this->renderContext($message, $data, $line_map); } } @@ -75,153 +75,148 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { protected function renderContext( ArcanistLintMessage $message, - array $line_data) { + $data, + array $line_map) { + + $context = 3; + + $message = $message->newTrimmedMessage(); + + $original = $message->getOriginalText(); + $replacement = $message->getReplacementText(); + + $line = $message->getLine(); + $char = $message->getChar(); + + $old = $data; + $old_lines = phutil_split_lines($old); + + if ($message->isPatchable()) { + $patch_offset = $line_map[$line] + ($char - 1); + + $new = substr_replace( + $old, + $replacement, + $patch_offset, + strlen($original)); + $new_lines = phutil_split_lines($new); + + // Figure out how many "-" and "+" lines we have by counting the newlines + // for the relevant patches. This may overestimate things if we are adding + // or removing entire lines, but we'll adjust things below. + $old_impact = substr_count($original, "\n") + 1; + $new_impact = substr_count($replacement, "\n") + 1; + + $start = $line; + + // If lines at the beginning of the changed line range are actually the + // same, shrink the range. This happens when a patch just adds a line. + do { + if ($old_lines[$start - 1] != $new_lines[$start - 1]) { + break; + } + + $start++; + $old_impact--; + $new_impact--; + + if ($old_impact < 0 || $new_impact < 0) { + throw new Exception( + pht( + 'Modified prefix line range has become negative '. + '(old = %d, new = %d).', + $old_impact, + $new_impact)); + } + } while (true); + + // If the lines at the end of the changed line range are actually the + // same, shrink the range. This happens when a patch just removes a + // line. + do { + $old_suffix = $old_lines[$start + $old_impact - 2]; + $new_suffix = $new_lines[$start + $new_impact - 2]; + + if ($old_suffix != $new_suffix) { + break; + } + + $old_impact--; + $new_impact--; + + if ($old_impact < 0 || $new_impact < 0) { + throw new Exception( + pht( + 'Modified suffix line range has become negative '. + '(old = %d, new = %d).', + $old_impact, + $new_impact)); + } + } while (true); + + } else { + $old_impact = 0; + $new_impact = 0; + } - $lines_of_context = 3; $out = array(); - $num_lines = count($line_data); - // make line numbers line up with array indexes - array_unshift($line_data, ''); - - $line_num = min($message->getLine(), $num_lines); - $line_num = max(1, $line_num); - - // Print out preceding context before the impacted region. - $cursor = max(1, $line_num - $lines_of_context); - for (; $cursor < $line_num; $cursor++) { - $out[] = $this->renderLine($cursor, $line_data[$cursor]); + $head = max(1, $start - $context); + for ($ii = $head; $ii < $start; $ii++) { + $out[] = array( + 'text' => $old_lines[$ii - 1], + 'number' => $ii, + ); } - $text = $message->getOriginalText(); - $start = $message->getChar() - 1; - $patch = ''; - // Refine original and replacement text to eliminate start and end in common - if ($message->isPatchable()) { - $patch = $message->getReplacementText(); - $text_strlen = strlen($text); - $patch_strlen = strlen($patch); - $min_length = min($text_strlen, $patch_strlen); - - $same_at_front = 0; - for ($ii = 0; $ii < $min_length; $ii++) { - if ($text[$ii] !== $patch[$ii]) { - break; - } - $same_at_front++; - $start++; - if ($text[$ii] == "\n") { - $out[] = $this->renderLine($cursor, $line_data[$cursor]); - $cursor++; - $start = 0; - $line_num++; - } - } - // deal with shorter string ' ' longer string ' a ' - $min_length -= $same_at_front; - - // And check the end of the string - $same_at_end = 0; - for ($ii = 1; $ii <= $min_length; $ii++) { - if ($text[$text_strlen - $ii] !== $patch[$patch_strlen - $ii]) { - break; - } - $same_at_end++; - } - - $text = substr( - $text, - $same_at_front, - $text_strlen - $same_at_end - $same_at_front); - $patch = substr( - $patch, - $same_at_front, - $patch_strlen - $same_at_end - $same_at_front); - } - // Print out the impacted region itself. - $diff = $message->isPatchable() ? '-' : null; - - $text_lines = explode("\n", $text); - $text_length = count($text_lines); - - $intraline = ($text != '' || $start || !preg_match('/\n$/', $patch)); - - if ($intraline) { - for (; $cursor < $line_num + $text_length; $cursor++) { - $chevron = ($cursor == $line_num); - // We may not have any data if, e.g., the old file does not exist. - $data = idx($line_data, $cursor, null); - - // Highlight the problem substring. - $text_line = $text_lines[$cursor - $line_num]; - if (strlen($text_line)) { - $data = substr_replace( - $data, - phutil_console_format('##%s##', $text_line), - ($cursor == $line_num ? ($start > 0 ? $start : null) : 0), - strlen($text_line)); - } - - $out[] = $this->renderLine($cursor, $data, $chevron, $diff); - } + for ($ii = $start; $ii < $start + $old_impact; $ii++) { + $out[] = array( + 'text' => $old_lines[$ii - 1], + 'number' => $ii, + 'type' => '-', + 'chevron' => ($ii == $start), + ); } - // Print out replacement text. - if ($message->isPatchable()) { - // Strip trailing newlines, since "explode" will create an extra patch - // line for these. - if (strlen($patch) && ($patch[strlen($patch) - 1] === "\n")) { - $patch = substr($patch, 0, -1); - } - $patch_lines = explode("\n", $patch); - $patch_length = count($patch_lines); - - $patch_line = $patch_lines[0]; - - $len = isset($text_lines[0]) ? strlen($text_lines[0]) : 0; - - $patched = phutil_console_format('##%s##', $patch_line); - - if ($intraline) { - $patched = substr_replace( - $line_data[$line_num], - $patched, - $start, - $len); - } - - $out[] = $this->renderLine(null, $patched, false, '+'); - - foreach (array_slice($patch_lines, 1) as $patch_line) { - $out[] = $this->renderLine( - null, - phutil_console_format('##%s##', $patch_line), false, '+'); - } + for ($ii = $start; $ii < $start + $new_impact; $ii++) { + $out[] = array( + 'text' => $new_lines[$ii - 1], + 'type' => '+', + 'chevron' => ($ii == $start), + ); } - $end = min($num_lines, $cursor + $lines_of_context); - for (; $cursor < $end; $cursor++) { - // If there is no original text, we didn't print out a chevron or any - // highlighted text above, so print it out here. This allows messages - // which don't have any original/replacement information to still - // render with indicator chevrons. - if ($text || $message->isPatchable()) { + $cursor = $start + $old_impact; + $foot = min(count($old_lines), $cursor + $context); + for ($ii = $cursor; $ii <= $foot; $ii++) { + $out[] = array( + 'text' => $old_lines[$ii - 1], + 'number' => $ii, + 'chevron' => ($ii == $cursor), + ); + } + + $result = array(); + + $seen_chevron = false; + foreach ($out as $spec) { + if ($seen_chevron) { $chevron = false; } else { - $chevron = ($cursor == $line_num); + $chevron = !empty($spec['chevron']); + if ($chevron) { + $seen_chevron = true; + } } - $out[] = $this->renderLine($cursor, $line_data[$cursor], $chevron); - // With original text, we'll render the text highlighted above. If the - // lint message only has a line/char offset there's nothing to - // highlight, so print out a caret on the next line instead. - if ($chevron && $message->getChar()) { - $out[] = $this->renderCaret($message->getChar()); - } + $result[] = $this->renderLine( + idx($spec, 'number'), + $spec['text'], + $chevron, + idx($spec, 'type')); } - $out[] = null; - return implode("\n", $out); + return implode('', $result); } private function renderCaret($pos) { @@ -245,4 +240,20 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { pht('No lint warnings.')); } + private function newOffsetMap($data) { + $lines = phutil_split_lines($data); + + $line_map = array(); + + $number = 1; + $offset = 0; + foreach ($lines as $line) { + $line_map[$number] = $offset; + $number++; + $offset += strlen($line); + } + + return $line_map; + } + } diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php index 320c00fb..7f39d352 100644 --- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php +++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php @@ -26,6 +26,34 @@ final class ArcanistConsoleLintRendererTestCase 'original' => 'tantawount', 'replacement' => 'tantamount', ), + + 'newline' => array( + 'line' => 6, + 'char' => 1, + 'original' => "\n", + 'replacement' => '', + ), + + 'addline' => array( + 'line' => 3, + 'char' => 1, + 'original' => '', + 'replacement' => "cherry\n", + ), + + 'addlinesuffix' => array( + 'line' => 2, + 'char' => 7, + 'original' => '', + 'replacement' => "\ncherry", + ), + + 'xml' => array( + 'line' => 3, + 'char' => 6, + 'original' => '', + 'replacement' => "\n", + ), ); $defaults = array( @@ -81,6 +109,11 @@ final class ArcanistConsoleLintRendererTestCase throw $ex; } + // Trim "~" off the ends of lines. This allows the "expect" file to test + // for trailing whitespace without actually containing trailing + // whitespace. + $expect = preg_replace('/~+$/m', '', $expect); + $this->assertEqual( $expect, $actual, diff --git a/src/lint/renderer/__tests__/data/addline.expect b/src/lint/renderer/__tests__/data/addline.expect new file mode 100644 index 00000000..44aa945c --- /dev/null +++ b/src/lint/renderer/__tests__/data/addline.expect @@ -0,0 +1,12 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 1 apple + 2 banana + >>> + cherry + 3 date + 4 eclaire + 5 fig diff --git a/src/lint/renderer/__tests__/data/addline.txt b/src/lint/renderer/__tests__/data/addline.txt new file mode 100644 index 00000000..07147513 --- /dev/null +++ b/src/lint/renderer/__tests__/data/addline.txt @@ -0,0 +1,5 @@ +apple +banana +date +eclaire +fig diff --git a/src/lint/renderer/__tests__/data/addlinesuffix.expect b/src/lint/renderer/__tests__/data/addlinesuffix.expect new file mode 100644 index 00000000..44aa945c --- /dev/null +++ b/src/lint/renderer/__tests__/data/addlinesuffix.expect @@ -0,0 +1,12 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 1 apple + 2 banana + >>> + cherry + 3 date + 4 eclaire + 5 fig diff --git a/src/lint/renderer/__tests__/data/addlinesuffix.txt b/src/lint/renderer/__tests__/data/addlinesuffix.txt new file mode 100644 index 00000000..07147513 --- /dev/null +++ b/src/lint/renderer/__tests__/data/addlinesuffix.txt @@ -0,0 +1,5 @@ +apple +banana +date +eclaire +fig diff --git a/src/lint/renderer/__tests__/data/newline.expect b/src/lint/renderer/__tests__/data/newline.expect new file mode 100644 index 00000000..0533707c --- /dev/null +++ b/src/lint/renderer/__tests__/data/newline.expect @@ -0,0 +1,14 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 3 ccc + 4 ddd + 5 eee + >>> - 6 ~ + 7 fff + 8 ggg + 9 hhh + 10 iii diff --git a/src/lint/renderer/__tests__/data/newline.txt b/src/lint/renderer/__tests__/data/newline.txt new file mode 100644 index 00000000..830c73a1 --- /dev/null +++ b/src/lint/renderer/__tests__/data/newline.txt @@ -0,0 +1,11 @@ +aaa +bbb +ccc +ddd +eee + +fff +ggg +hhh +iii +jjj diff --git a/src/lint/renderer/__tests__/data/xml.expect b/src/lint/renderer/__tests__/data/xml.expect new file mode 100644 index 00000000..767ea655 --- /dev/null +++ b/src/lint/renderer/__tests__/data/xml.expect @@ -0,0 +1,12 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 1 < + 2 wow + >>> - 3 xml> + + xml + + > + 4 diff --git a/src/lint/renderer/__tests__/data/xml.txt b/src/lint/renderer/__tests__/data/xml.txt new file mode 100644 index 00000000..136a688e --- /dev/null +++ b/src/lint/renderer/__tests__/data/xml.txt @@ -0,0 +1,4 @@ +< + wow + xml> + From 213ed3ff15d71ac9f44b4435f857826be5a41705 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 31 Aug 2017 12:37:19 -0700 Subject: [PATCH 4/5] Restore the caret pointer ("^") for lint lines which only have a character offset Summary: Ref T9846. This was dropped when I refactored how things are rendered; restore it. Test Plan: Added unit tests. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9846 Differential Revision: https://secure.phabricator.com/D18511 --- src/lint/renderer/ArcanistConsoleLintRenderer.php | 11 +++++++++-- .../__tests__/ArcanistConsoleLintRendererTestCase.php | 7 +++++++ src/lint/renderer/__tests__/data/caret.expect | 11 +++++++++++ src/lint/renderer/__tests__/data/caret.txt | 4 ++++ 4 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 src/lint/renderer/__tests__/data/caret.expect create mode 100644 src/lint/renderer/__tests__/data/caret.txt diff --git a/src/lint/renderer/ArcanistConsoleLintRenderer.php b/src/lint/renderer/ArcanistConsoleLintRenderer.php index 20e79b2f..4acbb644 100644 --- a/src/lint/renderer/ArcanistConsoleLintRenderer.php +++ b/src/lint/renderer/ArcanistConsoleLintRenderer.php @@ -90,6 +90,7 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { $old = $data; $old_lines = phutil_split_lines($old); + $start = $line; if ($message->isPatchable()) { $patch_offset = $line_map[$line] + ($char - 1); @@ -107,8 +108,6 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { $old_impact = substr_count($original, "\n") + 1; $new_impact = substr_count($replacement, "\n") + 1; - $start = $line; - // If lines at the beginning of the changed line range are actually the // same, shrink the range. This happens when a patch just adds a line. do { @@ -214,6 +213,14 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { $spec['text'], $chevron, idx($spec, 'type')); + + // If this is just a message and does not have a patch, put a little + // caret underneath the line to point out where the issue is. + if ($chevron) { + if (!$message->isPatchable() && !strlen($original)) { + $result[] = $this->renderCaret($char)."\n"; + } + } } return implode('', $result); diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php index 7f39d352..693b1788 100644 --- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php +++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php @@ -54,6 +54,13 @@ final class ArcanistConsoleLintRendererTestCase 'original' => '', 'replacement' => "\n", ), + + 'caret' => array( + 'line' => 2, + 'char' => 13, + 'name' => 'Fruit Misinformation', + 'description' => 'Arguably untrue.', + ), ); $defaults = array( diff --git a/src/lint/renderer/__tests__/data/caret.expect b/src/lint/renderer/__tests__/data/caret.expect new file mode 100644 index 00000000..1bb26faa --- /dev/null +++ b/src/lint/renderer/__tests__/data/caret.expect @@ -0,0 +1,11 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Fruit Misinformation + Arguably untrue. + + 1 Apples are round. + >>> 2 Bananas are round. + ^ + 3 Cherries are round. + 4 Dates are round. diff --git a/src/lint/renderer/__tests__/data/caret.txt b/src/lint/renderer/__tests__/data/caret.txt new file mode 100644 index 00000000..a36a81cf --- /dev/null +++ b/src/lint/renderer/__tests__/data/caret.txt @@ -0,0 +1,4 @@ +Apples are round. +Bananas are round. +Cherries are round. +Dates are round. From ad8214456add127935b2a49119d25670d9ef7963 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 31 Aug 2017 12:50:06 -0700 Subject: [PATCH 5/5] Restore ANSI highlighting of lint sections to lint output Summary: Fixes T9846. This restores the last missing feature, ANSI highlighting of diff sections. Test Plan: Added a mode so we can actually test this stuff, activated that mode, wrote unit tests. Did a bunch of actual lint locally too and looked at it, all seemed sane. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9846 Differential Revision: https://secure.phabricator.com/D18512 --- .../renderer/ArcanistConsoleLintRenderer.php | 50 ++++++++++++++++++- .../ArcanistConsoleLintRendererTestCase.php | 9 +++- .../renderer/__tests__/data/inline.expect | 4 +- .../renderer/__tests__/data/original.expect | 7 +++ src/lint/renderer/__tests__/data/original.txt | 1 + .../renderer/__tests__/data/overlap.expect | 4 +- .../renderer/__tests__/data/simple.expect | 4 +- 7 files changed, 71 insertions(+), 8 deletions(-) create mode 100644 src/lint/renderer/__tests__/data/original.expect create mode 100644 src/lint/renderer/__tests__/data/original.txt diff --git a/src/lint/renderer/ArcanistConsoleLintRenderer.php b/src/lint/renderer/ArcanistConsoleLintRenderer.php index 4acbb644..8880fbe8 100644 --- a/src/lint/renderer/ArcanistConsoleLintRenderer.php +++ b/src/lint/renderer/ArcanistConsoleLintRenderer.php @@ -6,12 +6,22 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { private $showAutofixPatches = false; + private $testableMode; public function setShowAutofixPatches($show_autofix_patches) { $this->showAutofixPatches = $show_autofix_patches; return $this; } + public function setTestableMode($testable_mode) { + $this->testableMode = $testable_mode; + return $this; + } + + public function getTestableMode() { + return $this->testableMode; + } + public function renderLintResult(ArcanistLintResult $result) { $messages = $result->getMessages(); $path = $result->getPath(); @@ -90,6 +100,7 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { $old = $data; $old_lines = phutil_split_lines($old); + $old_impact = substr_count($original, "\n") + 1; $start = $line; if ($message->isPatchable()) { @@ -105,9 +116,26 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { // Figure out how many "-" and "+" lines we have by counting the newlines // for the relevant patches. This may overestimate things if we are adding // or removing entire lines, but we'll adjust things below. - $old_impact = substr_count($original, "\n") + 1; $new_impact = substr_count($replacement, "\n") + 1; + + // If this is a change on a single line, we'll try to highlight the + // changed character range to make it easier to pick out. + if ($old_impact === 1 && $new_impact === 1) { + $old_lines[$start - 1] = substr_replace( + $old_lines[$start - 1], + $this->highlightText($original), + $char - 1, + strlen($original)); + + $new_lines[$start - 1] = substr_replace( + $new_lines[$start - 1], + $this->highlightText($replacement), + $char - 1, + strlen($replacement)); + } + + // If lines at the beginning of the changed line range are actually the // same, shrink the range. This happens when a patch just adds a line. do { @@ -154,6 +182,18 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { } while (true); } else { + + // If we have "original" text and it is contained on a single line, + // highlight the affected area. If we don't have any text, we'll mark + // the character with a caret (below, in rendering) instead. + if ($old_impact == 1 && strlen($original)) { + $old_lines[$start - 1] = substr_replace( + $old_lines[$start - 1], + $this->highlightText($original), + $char - 1, + strlen($original)); + } + $old_impact = 0; $new_impact = 0; } @@ -263,4 +303,12 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { return $line_map; } + private function highlightText($text) { + if ($this->getTestableMode()) { + return '>'.$text.'<'; + } else { + return (string)tsprintf('##%s##', $text); + } + } + } diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php index 693b1788..68a35420 100644 --- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php +++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php @@ -61,6 +61,12 @@ final class ArcanistConsoleLintRendererTestCase 'name' => 'Fruit Misinformation', 'description' => 'Arguably untrue.', ), + + 'original' => array( + 'line' => 1, + 'char' => 4, + 'original' => 'should of', + ), ); $defaults = array( @@ -105,7 +111,8 @@ final class ArcanistConsoleLintRendererTestCase ->setData($data) ->addMessage($message); - $renderer = new ArcanistConsoleLintRenderer(); + $renderer = id(new ArcanistConsoleLintRenderer()) + ->setTestableMode(true); try { PhutilConsoleFormatter::disableANSI(true); diff --git a/src/lint/renderer/__tests__/data/inline.expect b/src/lint/renderer/__tests__/data/inline.expect index eba4522b..d1d63fda 100644 --- a/src/lint/renderer/__tests__/data/inline.expect +++ b/src/lint/renderer/__tests__/data/inline.expect @@ -4,5 +4,5 @@ Warning (WARN123) Lint Warning Consider this. - >>> - 1 adjudicated - + adjudidoged + >>> - 1 adjudi>catdog>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + >>> 1 He >should of< known. diff --git a/src/lint/renderer/__tests__/data/original.txt b/src/lint/renderer/__tests__/data/original.txt new file mode 100644 index 00000000..090fe16d --- /dev/null +++ b/src/lint/renderer/__tests__/data/original.txt @@ -0,0 +1 @@ +He should of known. diff --git a/src/lint/renderer/__tests__/data/overlap.expect b/src/lint/renderer/__tests__/data/overlap.expect index 19de1c46..d7756f3d 100644 --- a/src/lint/renderer/__tests__/data/overlap.expect +++ b/src/lint/renderer/__tests__/data/overlap.expect @@ -4,5 +4,5 @@ Warning (WARN123) Lint Warning Consider this. - >>> - 1 tantawount - + tantamount + >>> - 1 tanta>wm>> - 1 a - + z + >>> - 1 >a< + + >z< 2 b 3 c