Render lint results as Harbormaster lint messages

Summary:
Ref T8095. Render lint results in a future-ready way.

This makes the renderer accept `HarbormasterBuildLintMessage` objects. If we have legacy data instead, it converts it into `HarbormasterBuildLintMessage` objects.

Design is a bit rough but will be cleaned up later after T7739.

This moves away from "postponed linters", which are obsolete after Harbormaster (and were only ever used by Facebook).

Test Plan: {F523429}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8095

Differential Revision: https://secure.phabricator.com/D13377

src/__phutil_library_map__.php

@@ -891,6 +891,7 @@ phutil_register_library_map(array(
     'HarbormasterDAO' => 'applications/harbormaster/storage/HarbormasterDAO.php',
     'HarbormasterHTTPRequestBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php',
     'HarbormasterLeaseHostBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterLeaseHostBuildStepImplementation.php',
+    'HarbormasterLintPropertyView' => 'applications/harbormaster/view/HarbormasterLintPropertyView.php',
     'HarbormasterManagePlansCapability' => 'applications/harbormaster/capability/HarbormasterManagePlansCapability.php',
     'HarbormasterManagementBuildWorkflow' => 'applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php',
     'HarbormasterManagementUpdateWorkflow' => 'applications/harbormaster/management/HarbormasterManagementUpdateWorkflow.php',
@@ -4351,6 +4352,7 @@ phutil_register_library_map(array(
     'HarbormasterDAO' => 'PhabricatorLiskDAO',
     'HarbormasterHTTPRequestBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
     'HarbormasterLeaseHostBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
+    'HarbormasterLintPropertyView' => 'AphrontView',
     'HarbormasterManagePlansCapability' => 'PhabricatorPolicyCapability',
     'HarbormasterManagementBuildWorkflow' => 'HarbormasterManagementWorkflow',
     'HarbormasterManagementUpdateWorkflow' => 'HarbormasterManagementWorkflow',

src/applications/differential/customfield/DifferentialLintField.php

@@ -38,7 +38,6 @@ final class DifferentialLintField
     $keys = array(
       'arc:lint',
       'arc:lint-excuse',
-      'arc:lint-postponed',
     );
 
     $properties = id(new DifferentialDiffProperty())->loadAllWhere(
@@ -51,208 +50,57 @@ final class DifferentialLintField
       $diff->attachProperty($key, idx($properties, $key));
     }
 
-    $path_changesets = mpull($diff->loadChangesets(), 'getID', 'getFilename');
+    $status = $this->renderLintStatus($diff);
 
-    $lstar = DifferentialRevisionUpdateHistoryView::renderDiffLintStar($diff);
+    $lint = array();
-    $lmsg = DifferentialRevisionUpdateHistoryView::getDiffLintMessage($diff);
-    $ldata = $diff->getProperty('arc:lint');
-    $ltail = null;
 
-    $rows = array();
+    // TODO: Look for Harbormaster messages here.
 
-    $rows[] = array(
-      'style'     => 'star',
-      'name'      => $lstar,
-      'value'     => $lmsg,
-      'show'      => true,
-    );
 
-    $excuse = $diff->getProperty('arc:lint-excuse');
+    if (!$lint) {
-    if ($excuse) {
+      // No Harbormaster messages, so look for legacy messages and make them
-      $rows[] = array(
+      // look like modern messages.
-        'style'   => 'excuse',
+      $legacy_lint = $diff->getProperty('arc:lint');
-        'name'    => 'Excuse',
+      if ($legacy_lint) {
-        'value'   => phutil_escape_html_newlines($excuse),
+        // Show the top 100 legacy lint messages. Previously, we showed some
-        'show'    => true,
+        // by default and let the user toggle the rest. With modern messages,
-      );
+        // we can send the user to the Harbormaster detail page. Just show
+        // "a lot" of messages in legacy cases to try to strike a balance
+        // between implementation simplicitly and compatibility.
+        $legacy_lint = array_slice($legacy_lint, 0, 100);
+
+        $target = new HarbormasterBuildTarget();
+        foreach ($legacy_lint as $message) {
+          $modern = HarbormasterBuildLintMessage::newFromDictionary(
+            $target,
+            $this->getModernLintMessageDictionary($message));
+          $lint[] = $modern;
+        }
+      }
     }
 
-    $show_limit = 10;
+    if ($lint) {
-    $hidden = array();
+      $path_map = mpull($diff->loadChangesets(), 'getID', 'getFilename');
+      foreach ($path_map as $path => $id) {
+        $href = '#C'.$id.'NL';
 
-    if ($ldata) {
+        // TODO: When the diff is not the right-hand-size diff, we should
-      $ldata = igroup($ldata, 'path');
+        // ideally adjust this URI to be absolute.
-      foreach ($ldata as $path => $messages) {
 
-        $rows[] = array(
+        $path_map[$path] = $href;
-          'style' => 'section',
-          'name'  => $path,
-          'show'  => $show_limit,
-        );
-
-        foreach ($messages as $message) {
-          $path = idx($message, 'path');
-          $line = idx($message, 'line');
-
-          $code = idx($message, 'code');
-          $severity = idx($message, 'severity');
-
-          $name = idx($message, 'name');
-          $description = idx($message, 'description');
-
-          $line_link = pht('line %d', intval($line));
-          if (isset($path_changesets[$path])) {
-            $href = '#C'.$path_changesets[$path].'NL'.max(1, $line);
-
-            // TODO: We are always showing the active diff
-            // if ($diff->getID() != $this->getDiff()->getID()) {
-            //  $href = '/D'.$diff->getRevisionID().'?id='.$diff->getID().$href;
-            // }
-
-            $line_link = phutil_tag(
-              'a',
-              array(
-                'href' => $href,
-              ),
-              $line_link);
       }
 
-          if ($show_limit) {
+      $view = id(new HarbormasterLintPropertyView())
-            --$show_limit;
+        ->setPathURIMap($path_map)
-            $show = true;
+        ->setLintMessages($lint);
     } else {
-            $show = false;
+      $view = null;
-            if (empty($hidden[$severity])) {
-              $hidden[$severity] = 0;
-            }
-            $hidden[$severity]++;
     }
 
-          $rows[] = array(
+    return array(
-            'style' => $this->getSeverityStyle($severity),
+      $status,
-            'name'  => ucwords($severity),
+      $view,
-            'value' => hsprintf(
-              '(%s) %s at %s',
-              $code,
-              $name,
-              $line_link),
-            'show'  => $show,
     );
-
-          if (!empty($message['locations'])) {
-            $locations = array();
-            foreach ($message['locations'] as $location) {
-              $other_line = idx($location, 'line');
-              $locations[] =
-                idx($location, 'path', $path).
-                ($other_line ? ":{$other_line}" : '');
-            }
-            $description .= "\n".pht(
-              'Other locations: %s',
-              implode(', ', $locations));
-          }
-
-          if (strlen($description)) {
-            $rows[] = array(
-              'style' => 'details',
-              'value' => phutil_escape_html_newlines($description),
-              'show'  => false,
-            );
-            if (empty($hidden['details'])) {
-              $hidden['details'] = 0;
-            }
-            $hidden['details']++;
-          }
-        }
-      }
-    }
-
-    $postponed = $diff->getProperty('arc:lint-postponed');
-    if ($postponed) {
-      foreach ($postponed as $linter) {
-        $rows[] = array(
-          'style' => $this->getPostponedStyle(),
-          'name' => 'Postponed',
-          'value' => $linter,
-          'show'  => false,
-          );
-        if (empty($hidden['postponed'])) {
-          $hidden['postponed'] = 0;
-        }
-        $hidden['postponed']++;
-      }
-    }
-
-    $show_string = $this->renderShowString($hidden);
-
-    $view = new DifferentialResultsTableView();
-    $view->setRows($rows);
-    $view->setShowMoreString($show_string);
-
-    return $view->render();
-  }
-
-  private function getSeverityStyle($severity) {
-    $map = array(
-      ArcanistLintSeverity::SEVERITY_ERROR      => 'red',
-      ArcanistLintSeverity::SEVERITY_WARNING    => 'yellow',
-      ArcanistLintSeverity::SEVERITY_AUTOFIX    => 'yellow',
-      ArcanistLintSeverity::SEVERITY_ADVICE     => 'yellow',
-    );
-    return idx($map, $severity);
-  }
-
-  private function getPostponedStyle() {
-    return 'blue';
-  }
-
-  private function renderShowString(array $hidden) {
-    if (!$hidden) {
-      return null;
-    }
-
-    // Reorder hidden things by severity.
-    $hidden = array_select_keys(
-      $hidden,
-      array(
-        ArcanistLintSeverity::SEVERITY_ERROR,
-        ArcanistLintSeverity::SEVERITY_WARNING,
-        ArcanistLintSeverity::SEVERITY_AUTOFIX,
-        ArcanistLintSeverity::SEVERITY_ADVICE,
-        'details',
-        'postponed',
-      )) + $hidden;
-
-    $show = array();
-    foreach ($hidden as $key => $value) {
-      switch ($key) {
-        case ArcanistLintSeverity::SEVERITY_ERROR:
-          $show[] = pht('%d Error(s)', $value);
-          break;
-        case ArcanistLintSeverity::SEVERITY_WARNING:
-          $show[] = pht('%d Warning(s)', $value);
-          break;
-        case ArcanistLintSeverity::SEVERITY_AUTOFIX:
-          $show[] = pht('%d Auto-Fix(es)', $value);
-          break;
-        case ArcanistLintSeverity::SEVERITY_ADVICE:
-          $show[] = pht('%d Advice(s)', $value);
-          break;
-        case 'details':
-          $show[] = pht('%d Detail(s)', $value);
-          break;
-        case 'postponed':
-          $show[] = pht('%d Postponed', $value);
-          break;
-        default:
-          $show[] = $value;
-          break;
-      }
-    }
-
-    return pht(
-      'Show Full Lint Results (%s)',
-      implode(', ', $show));
   }
 
   public function getWarningsForDetailView() {
@@ -278,4 +126,43 @@ final class DifferentialLintField
     return $warnings;
   }
 
+  private function renderLintStatus(DifferentialDiff $diff) {
+    $colors = array(
+      DifferentialLintStatus::LINT_NONE => 'grey',
+      DifferentialLintStatus::LINT_OKAY => 'green',
+      DifferentialLintStatus::LINT_WARN => 'yellow',
+      DifferentialLintStatus::LINT_FAIL => 'red',
+      DifferentialLintStatus::LINT_SKIP => 'blue',
+      DifferentialLintStatus::LINT_AUTO_SKIP => 'blue',
+      DifferentialLintStatus::LINT_POSTPONED => 'blue',
+    );
+    $icon_color = idx($colors, $diff->getLintStatus(), 'grey');
+
+    $message = DifferentialRevisionUpdateHistoryView::getDiffLintMessage($diff);
+
+    $excuse = $diff->getProperty('arc:lint-excuse');
+    if (strlen($excuse)) {
+      $excuse = array(
+        phutil_tag('strong', array(), pht('Excuse:')),
+        ' ',
+        phutil_escape_html_newlines($excuse),
+      );
+    }
+
+    $status = id(new PHUIStatusListView())
+      ->addItem(
+        id(new PHUIStatusItemView())
+          ->setIcon(PHUIStatusItemView::ICON_STAR, $icon_color)
+          ->setTarget($message)
+          ->setNote($excuse));
+
+    return $status;
+  }
+
+  private function getModernLintMessageDictionary(array $map) {
+    // TODO: We might need to remap some stuff here?
+    return $map;
+  }
+
+
 }

src/applications/harbormaster/view/HarbormasterLintPropertyView.php

@@ -0,0 +1,78 @@
+<?php
+
+final class HarbormasterLintPropertyView extends AphrontView {
+
+  private $pathURIMap;
+  private $lintMessages = array();
+
+  public function setPathURIMap(array $map) {
+    $this->pathURIMap = $map;
+    return $this;
+  }
+
+  public function setLintMessages(array $messages) {
+    assert_instances_of($messages, 'HarbormasterBuildLintMessage');
+    $this->lintMessages = $messages;
+    return $this;
+  }
+
+  public function render() {
+    $rows = array();
+    foreach ($this->lintMessages as $message) {
+      $path = $message->getPath();
+      $line = $message->getLine();
+
+      $href = null;
+      if (strlen(idx($this->pathURIMap, $path))) {
+        $href = $this->pathURIMap[$path].max($line, 1);
+      }
+
+      $severity = $this->renderSeverity($message->getSeverity());
+
+      $location = $path.':'.$line;
+      if (strlen($href)) {
+        $location = phutil_tag(
+          'a',
+          array(
+            'href' => $href,
+          ),
+          $location);
+      }
+
+      $rows[] = array(
+        $location,
+        $severity,
+        $message->getCode(),
+        $message->getName(),
+      );
+    }
+
+    $table = id(new AphrontTableView($rows))
+      ->setHeaders(
+        array(
+          pht('Location'),
+          pht('Severity'),
+          pht('Code'),
+          pht('Message'),
+        ))
+      ->setColumnClasses(
+        array(
+          'pri',
+          null,
+          null,
+          'wide',
+        ));
+
+    return $table;
+  }
+
+  private function renderSeverity($severity) {
+    $names = ArcanistLintSeverity::getLintSeverities();
+    $name = idx($names, $severity, $severity);
+
+    // TODO: Add some color here?
+
+    return $name;
+  }
+
+}

src/view/phui/PHUIStatusItemView.php

@@ -22,6 +22,7 @@ final class PHUIStatusItemView extends AphrontTagView {
   const ICON_MINUS = 'fa-minus-circle';
   const ICON_OPEN = 'fa-circle-o';
   const ICON_CLOCK = 'fa-clock-o';
+  const ICON_STAR = 'fa-star';
 
   /* render_textarea */
   public function setIcon($icon, $color = null, $label = null) {