Skip to content

Commit 1022cfb

Browse files
committed
Remove GitHub comment footer.
These footnotes make the comment longer but they feel very low value to me.
1 parent c305870 commit 1022cfb

File tree

2 files changed

+52
-71
lines changed

2 files changed

+52
-71
lines changed

site/src/comparison.rs

Lines changed: 46 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ pub async fn handle_triage(
9595
.clone()
9696
.summarize_by_category(&benchmark_map);
9797
let mut result = String::from("**Summary**:\n\n");
98-
write_summary_table(&primary, &secondary, false, true, &mut result);
98+
write_summary_table(&primary, &secondary, true, &mut result);
9999
result
100100
}
101101
None => String::from("**ERROR**: no data found for end bound"),
@@ -547,7 +547,7 @@ async fn write_triage_summary(
547547
let link = &compare_link(start, end);
548548
write!(&mut result, " [(Comparison Link)]({})\n\n", link).unwrap();
549549

550-
write_summary_table(&primary, &secondary, false, true, &mut result);
550+
write_summary_table(&primary, &secondary, true, &mut result);
551551

552552
result
553553
}
@@ -556,7 +556,6 @@ async fn write_triage_summary(
556556
pub fn write_summary_table(
557557
primary: &ArtifactComparisonSummary,
558558
secondary: &ArtifactComparisonSummary,
559-
with_footnotes: bool,
560559
include_metric: bool,
561560
result: &mut String,
562561
) {
@@ -668,9 +667,9 @@ pub fn write_summary_table(
668667
// easy to read for anyone who is viewing the Markdown source.
669668
let column_labels = [
670669
metric,
671-
format!("mean{}", if with_footnotes { "[^1]" } else { "" }),
670+
format!("mean"),
672671
"range".to_string(),
673-
format!("count{}", if with_footnotes { "[^2]" } else { "" }),
672+
format!("count"),
674673
];
675674
let counts: Vec<usize> = column_labels.iter().map(|s| s.chars().count()).collect();
676675
for column in &column_labels {
@@ -692,16 +691,6 @@ pub fn write_summary_table(
692691
}
693692
}
694693

695-
pub fn write_summary_table_footer(result: &mut String) {
696-
writeln!(
697-
result,
698-
r#"
699-
[^1]: *the arithmetic mean of the percent change*
700-
[^2]: *number of relevant changes*"#
701-
)
702-
.unwrap();
703-
}
704-
705694
/// Compare two bounds on a given stat
706695
///
707696
/// Returns Ok(None) when no data for the end bound is present
@@ -1437,11 +1426,11 @@ mod tests {
14371426
(Category::Primary, 1.0, 3.0),
14381427
],
14391428
r#"
1440-
| Regressions ❌ <br /> (primary) | 146.7% | [100.0%, 200.0%] | 3 |
1441-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1442-
| Improvements ✅ <br /> (primary) | - | - | 0 |
1443-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1444-
| All ❌✅ (primary) | 146.7% | [100.0%, 200.0%] | 3 |
1429+
| Regressions ❌ <br /> (primary) | 146.7% | [100.0%, 200.0%] | 3 |
1430+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1431+
| Improvements ✅ <br /> (primary) | - | - | 0 |
1432+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1433+
| All ❌✅ (primary) | 146.7% | [100.0%, 200.0%] | 3 |
14451434
"#
14461435
.trim_start(),
14471436
);
@@ -1456,11 +1445,11 @@ mod tests {
14561445
(Category::Primary, 4.0, 1.0),
14571446
],
14581447
r#"
1459-
| Regressions ❌ <br /> (primary) | - | - | 0 |
1460-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1461-
| Improvements ✅ <br /> (primary) | -71.7% | [-80.0%, -60.0%] | 3 |
1462-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1463-
| All ❌✅ (primary) | -71.7% | [-80.0%, -60.0%] | 3 |
1448+
| Regressions ❌ <br /> (primary) | - | - | 0 |
1449+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1450+
| Improvements ✅ <br /> (primary) | -71.7% | [-80.0%, -60.0%] | 3 |
1451+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1452+
| All ❌✅ (primary) | -71.7% | [-80.0%, -60.0%] | 3 |
14641453
"#
14651454
.trim_start(),
14661455
);
@@ -1475,11 +1464,11 @@ mod tests {
14751464
(Category::Secondary, 4.0, 1.0),
14761465
],
14771466
r#"
1478-
| Regressions ❌ <br /> (primary) | - | - | 0 |
1479-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1480-
| Improvements ✅ <br /> (primary) | - | - | 0 |
1481-
| Improvements ✅ <br /> (secondary) | -71.7% | [-80.0%, -60.0%] | 3 |
1482-
| All ❌✅ (primary) | - | - | 0 |
1467+
| Regressions ❌ <br /> (primary) | - | - | 0 |
1468+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1469+
| Improvements ✅ <br /> (primary) | - | - | 0 |
1470+
| Improvements ✅ <br /> (secondary) | -71.7% | [-80.0%, -60.0%] | 3 |
1471+
| All ❌✅ (primary) | - | - | 0 |
14831472
"#
14841473
.trim_start(),
14851474
);
@@ -1494,11 +1483,11 @@ mod tests {
14941483
(Category::Secondary, 1.0, 3.0),
14951484
],
14961485
r#"
1497-
| Regressions ❌ <br /> (primary) | - | - | 0 |
1498-
| Regressions ❌ <br /> (secondary) | 146.7% | [100.0%, 200.0%] | 3 |
1499-
| Improvements ✅ <br /> (primary) | - | - | 0 |
1500-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1501-
| All ❌✅ (primary) | - | - | 0 |
1486+
| Regressions ❌ <br /> (primary) | - | - | 0 |
1487+
| Regressions ❌ <br /> (secondary) | 146.7% | [100.0%, 200.0%] | 3 |
1488+
| Improvements ✅ <br /> (primary) | - | - | 0 |
1489+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1490+
| All ❌✅ (primary) | - | - | 0 |
15021491
"#
15031492
.trim_start(),
15041493
);
@@ -1514,11 +1503,11 @@ mod tests {
15141503
(Category::Primary, 4.0, 1.0),
15151504
],
15161505
r#"
1517-
| Regressions ❌ <br /> (primary) | 150.0% | [100.0%, 200.0%] | 2 |
1518-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1519-
| Improvements ✅ <br /> (primary) | -62.5% | [-75.0%, -50.0%] | 2 |
1520-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1521-
| All ❌✅ (primary) | 43.8% | [-75.0%, 200.0%] | 4 |
1506+
| Regressions ❌ <br /> (primary) | 150.0% | [100.0%, 200.0%] | 2 |
1507+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1508+
| Improvements ✅ <br /> (primary) | -62.5% | [-75.0%, -50.0%] | 2 |
1509+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1510+
| All ❌✅ (primary) | 43.8% | [-75.0%, 200.0%] | 4 |
15221511
"#
15231512
.trim_start(),
15241513
);
@@ -1536,11 +1525,11 @@ mod tests {
15361525
(Category::Primary, 4.0, 1.0),
15371526
],
15381527
r#"
1539-
| Regressions ❌ <br /> (primary) | 150.0% | [100.0%, 200.0%] | 2 |
1540-
| Regressions ❌ <br /> (secondary) | 100.0% | [100.0%, 100.0%] | 1 |
1541-
| Improvements ✅ <br /> (primary) | -62.5% | [-75.0%, -50.0%] | 2 |
1542-
| Improvements ✅ <br /> (secondary) | -66.7% | [-66.7%, -66.7%] | 1 |
1543-
| All ❌✅ (primary) | 43.8% | [-75.0%, 200.0%] | 4 |
1528+
| Regressions ❌ <br /> (primary) | 150.0% | [100.0%, 200.0%] | 2 |
1529+
| Regressions ❌ <br /> (secondary) | 100.0% | [100.0%, 100.0%] | 1 |
1530+
| Improvements ✅ <br /> (primary) | -62.5% | [-75.0%, -50.0%] | 2 |
1531+
| Improvements ✅ <br /> (secondary) | -66.7% | [-66.7%, -66.7%] | 1 |
1532+
| All ❌✅ (primary) | 43.8% | [-75.0%, 200.0%] | 4 |
15441533
"#
15451534
.trim_start(),
15461535
);
@@ -1554,11 +1543,11 @@ mod tests {
15541543
(Category::Primary, 5.0, 6.0),
15551544
],
15561545
r#"
1557-
| Regressions ❌ <br /> (primary) | 20.0% | [20.0%, 20.0%] | 1 |
1558-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1559-
| Improvements ✅ <br /> (primary) | -50.0% | [-50.0%, -50.0%] | 1 |
1560-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1561-
| All ❌✅ (primary) | -15.0% | [-50.0%, 20.0%] | 2 |
1546+
| Regressions ❌ <br /> (primary) | 20.0% | [20.0%, 20.0%] | 1 |
1547+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1548+
| Improvements ✅ <br /> (primary) | -50.0% | [-50.0%, -50.0%] | 1 |
1549+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1550+
| All ❌✅ (primary) | -15.0% | [-50.0%, 20.0%] | 2 |
15621551
"#
15631552
.trim_start(),
15641553
);
@@ -1572,11 +1561,11 @@ mod tests {
15721561
(Category::Primary, 6.0, 5.0),
15731562
],
15741563
r#"
1575-
| Regressions ❌ <br /> (primary) | 100.0% | [100.0%, 100.0%] | 1 |
1576-
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1577-
| Improvements ✅ <br /> (primary) | -16.7% | [-16.7%, -16.7%] | 1 |
1578-
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1579-
| All ❌✅ (primary) | 41.7% | [-16.7%, 100.0%] | 2 |
1564+
| Regressions ❌ <br /> (primary) | 100.0% | [100.0%, 100.0%] | 1 |
1565+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
1566+
| Improvements ✅ <br /> (primary) | -16.7% | [-16.7%, -16.7%] | 1 |
1567+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
1568+
| All ❌✅ (primary) | 41.7% | [-16.7%, 100.0%] | 2 |
15801569
"#
15811570
.trim_start(),
15821571
);
@@ -1625,8 +1614,8 @@ mod tests {
16251614
let secondary = ArtifactComparisonSummary::summarize(secondary_comparisons);
16261615

16271616
let mut result = String::new();
1628-
write_summary_table(&primary, &secondary, true, true, &mut result);
1629-
let header = "| (instructions:u) | mean[^1] | range | count[^2] |\n|:----------------:|:--------:|:-----:|:---------:|\n";
1617+
write_summary_table(&primary, &secondary, true, &mut result);
1618+
let header = "| (instructions:u) | mean | range | count |\n|:----------------:|:----:|:-----:|:-----:|\n";
16301619
assert_eq!(result, format!("{header}{expected}"));
16311620
}
16321621
}

site/src/github/comparison_summary.rs

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use crate::comparison::{
2-
deserves_attention_icount, write_summary_table, write_summary_table_footer, ArtifactComparison,
3-
ArtifactComparisonSummary, Direction, Metric,
2+
deserves_attention_icount, write_summary_table, ArtifactComparison, ArtifactComparisonSummary,
3+
Direction, Metric,
44
};
55
use crate::load::SiteCtxt;
66

@@ -174,7 +174,6 @@ async fn summarize_run(
174174
}
175175
}
176176

177-
let mut table_written = false;
178177
let metrics = vec![
179178
(
180179
"Instruction count",
@@ -203,45 +202,38 @@ async fn summarize_run(
203202
));
204203

205204
let (primary, secondary) = comparison.summarize_by_category(&benchmark_map);
206-
table_written |= write_metric_summary(primary, secondary, highly_reliable, &mut message);
207-
}
208-
209-
if table_written {
210-
write_summary_table_footer(&mut message);
205+
write_metric_summary(primary, secondary, highly_reliable, &mut message);
211206
}
212207

213208
Ok(message)
214209
}
215210

216-
/// Returns true if a summary table was written to `message`.
217211
fn write_metric_summary(
218212
primary: ArtifactComparisonSummary,
219213
secondary: ArtifactComparisonSummary,
220214
highly_reliable: bool,
221215
message: &mut String,
222-
) -> bool {
216+
) {
223217
if !primary.is_relevant() && !secondary.is_relevant() {
224218
message
225219
.push_str("This benchmark run did not return any relevant results for this metric.\n");
226-
false
227220
} else {
228221
if highly_reliable {
229222
message.push_str(
230223
"This is a highly reliable metric that was used to determine the \
231224
overall result at the top of this comment.\n\n",
232225
);
233-
write_summary_table(&primary, &secondary, true, false, message);
226+
write_summary_table(&primary, &secondary, false, message);
234227
} else {
235228
// `<details>` means it is hidden, requiring a click to reveal.
236229
message.push_str("<details>\n<summary>Results</summary>\n\n");
237230
message.push_str(
238231
"This is a less reliable metric that may be of interest but was not \
239232
used to determine the overall result at the top of this comment.\n\n",
240233
);
241-
write_summary_table(&primary, &secondary, true, false, message);
234+
write_summary_table(&primary, &secondary, false, message);
242235
message.push_str("</details>\n");
243236
}
244-
true
245237
}
246238
}
247239

0 commit comments

Comments
 (0)