Page MenuHomePhabricator
Files F13190638

D13377.diff
No OneTemporary
Actions

D13377.diff
View Options

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -891,6 +891,7 @@
'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 @@
'HarbormasterDAO' => 'PhabricatorLiskDAO',
'HarbormasterHTTPRequestBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
'HarbormasterLeaseHostBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
+ 'HarbormasterLintPropertyView' => 'AphrontView',
'HarbormasterManagePlansCapability' => 'PhabricatorPolicyCapability',
'HarbormasterManagementBuildWorkflow' => 'HarbormasterManagementWorkflow',
'HarbormasterManagementUpdateWorkflow' => 'HarbormasterManagementWorkflow',
diff --git a/src/applications/differential/customfield/DifferentialLintField.php b/src/applications/differential/customfield/DifferentialLintField.php
--- a/src/applications/differential/customfield/DifferentialLintField.php
+++ b/src/applications/differential/customfield/DifferentialLintField.php
@@ -38,7 +38,6 @@
$keys = array(
'arc:lint',
'arc:lint-excuse',
- 'arc:lint-postponed',
);
$properties = id(new DifferentialDiffProperty())->loadAllWhere(
@@ -51,208 +50,57 @@
$diff->attachProperty($key, idx($properties, $key));
}
- $path_changesets = mpull($diff->loadChangesets(), 'getID', 'getFilename');
+ $status = $this->renderLintStatus($diff);
- $lstar = DifferentialRevisionUpdateHistoryView::renderDiffLintStar($diff);
- $lmsg = DifferentialRevisionUpdateHistoryView::getDiffLintMessage($diff);
- $ldata = $diff->getProperty('arc:lint');
- $ltail = null;
+ $lint = array();
- $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 ($excuse) {
- $rows[] = array(
- 'style' => 'excuse',
- 'name' => 'Excuse',
- 'value' => phutil_escape_html_newlines($excuse),
- 'show' => true,
- );
- }
+ if (!$lint) {
+ // No Harbormaster messages, so look for legacy messages and make them
+ // look like modern messages.
+ $legacy_lint = $diff->getProperty('arc:lint');
+ if ($legacy_lint) {
+ // Show the top 100 legacy lint messages. Previously, we showed some
+ // 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);
- $show_limit = 10;
- $hidden = array();
-
- if ($ldata) {
- $ldata = igroup($ldata, 'path');
- foreach ($ldata as $path => $messages) {
-
- $rows[] = array(
- '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) {
- --$show_limit;
- $show = true;
- } else {
- $show = false;
- if (empty($hidden[$severity])) {
- $hidden[$severity] = 0;
- }
- $hidden[$severity]++;
- }
-
- $rows[] = array(
- 'style' => $this->getSeverityStyle($severity),
- 'name' => ucwords($severity),
- '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']++;
- }
+ $target = new HarbormasterBuildTarget();
+ foreach ($legacy_lint as $message) {
+ $modern = HarbormasterBuildLintMessage::newFromDictionary(
+ $target,
+ $this->getModernLintMessageDictionary($message));
+ $lint[] = $modern;
}
}
}
- $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);
+ if ($lint) {
+ $path_map = mpull($diff->loadChangesets(), 'getID', 'getFilename');
+ foreach ($path_map as $path => $id) {
+ $href = '#C'.$id.'NL';
- 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;
- }
+ // TODO: When the diff is not the right-hand-size diff, we should
+ // ideally adjust this URI to be absolute.
- // 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;
+ $path_map[$path] = $href;
}
+
+ $view = id(new HarbormasterLintPropertyView())
+ ->setPathURIMap($path_map)
+ ->setLintMessages($lint);
+ } else {
+ $view = null;
}
- return pht(
- 'Show Full Lint Results (%s)',
- implode(', ', $show));
+ return array(
+ $status,
+ $view,
+ );
}
public function getWarningsForDetailView() {
@@ -278,4 +126,43 @@
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;
+ }
+
+
}
diff --git a/src/applications/harbormaster/view/HarbormasterLintPropertyView.php b/src/applications/harbormaster/view/HarbormasterLintPropertyView.php
new file mode 100644
--- /dev/null
+++ b/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;
+ }
+
+}
diff --git a/src/view/phui/PHUIStatusItemView.php b/src/view/phui/PHUIStatusItemView.php
--- a/src/view/phui/PHUIStatusItemView.php
+++ b/src/view/phui/PHUIStatusItemView.php
@@ -22,6 +22,7 @@
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) {

File Metadata

Mime Type
text/plain
Expires
May 12 2024, 12:44 PM (5 w, 23 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6291274
Default Alt Text
D13377.diff (13 KB)

Event Timeline

Phabricator · Built by Phacility