mirror of
https://we.phorge.it/source/arcanist.git
synced 2024-11-26 00:32:41 +01:00
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
This commit is contained in:
parent
ab0d81bca2
commit
be67df6118
7 changed files with 171 additions and 4 deletions
|
@ -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',
|
||||
|
|
|
@ -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);
|
||||
}
|
||||
|
||||
}
|
||||
|
|
80
src/lint/__tests__/ArcanistLintMessageTestCase.php
Normal file
80
src/lint/__tests__/ArcanistLintMessageTestCase.php
Normal file
|
@ -0,0 +1,80 @@
|
|||
<?php
|
||||
|
||||
final class ArcanistLintMessageTestCase
|
||||
extends PhutilTestCase {
|
||||
|
||||
public function testMessageTrimming() {
|
||||
$map = array(
|
||||
'simple' => 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));
|
||||
}
|
||||
}
|
||||
}
|
File diff suppressed because one or more lines are too long
|
@ -17,6 +17,15 @@ final class ArcanistConsoleLintRendererTestCase
|
|||
'original' => 'cat',
|
||||
'replacement' => 'dog',
|
||||
),
|
||||
|
||||
// In this test, the original and replacement texts have a large
|
||||
// amount of overlap.
|
||||
'overlap' => array(
|
||||
'line' => 1,
|
||||
'char' => 1,
|
||||
'original' => 'tantawount',
|
||||
'replacement' => 'tantamount',
|
||||
),
|
||||
);
|
||||
|
||||
$defaults = array(
|
||||
|
|
8
src/lint/renderer/__tests__/data/overlap.expect
Normal file
8
src/lint/renderer/__tests__/data/overlap.expect
Normal file
|
@ -0,0 +1,8 @@
|
|||
>>> Lint for path/to/example.c:
|
||||
|
||||
|
||||
Warning (WARN123) Lint Warning
|
||||
Consider this.
|
||||
|
||||
>>> - 1 tantawount
|
||||
+ tantamount
|
1
src/lint/renderer/__tests__/data/overlap.txt
Normal file
1
src/lint/renderer/__tests__/data/overlap.txt
Normal file
|
@ -0,0 +1 @@
|
|||
tantawount
|
Loading…
Reference in a new issue