Skip to content

Align PR summary table to be more readable in its raw form #1312

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 2, 2022

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Apr 26, 2022

Aligns the columns to the same width (more or less), to be more readable in plain text. Let me know what you think before I update all the tests :)

Before:

| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
|:---:|:---:|:---:|:---:|:---:|:---:|
| count[^1] | 2 | 0 | 2 | 0 | 4 |
| mean[^2] | 150.0% | N/A | -62.5% | N/A | 43.8% |
| max | 200.0% | N/A | -75.0% | N/A | 200.0% |

[^1]: *number of relevant changes*
[^2]: *the arithmetic mean of the percent change*

After:

|            | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
|:----------:|:------------------------------:|:--------------------------------:|:-------------------------------:|:---------------------------------:|:------------------------:|
| count[^1]  | 2                              | 0                                | 2                               | 0                                 | 4                        |
| mean[^2]   | 150.0%                         | N/A                              | -62.5%                          | N/A                               | 43.8%                    |
| max        | 200.0%                         | N/A                              | -75.0%                          | N/A                               | 200.0%                   |

[^1]: *number of relevant changes*
[^2]: *the arithmetic mean of the percent change*

Fixes: #1310

@Kobzol Kobzol requested a review from rylev April 26, 2022 13:26
@klensy
Copy link
Contributor

klensy commented Apr 26, 2022

Maybe remove emoji from column titles?

When there was not a table (in previous version of reports), but lines with emoji, was used only a few ones when appropriate, but now there a lot of it unconditionally.

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 26, 2022

I realize that emojis are mostly useless for plain text, but at least for me it's quite useful in GitHub comments, as it allows me to quickly identify which columns are the regressions and which are the improvements.

Copy link
Member

@rylev rylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just need to fix the CI breakage.

@Kobzol Kobzol force-pushed the pr-summary-raw-alignment branch from e4da393 to c1f1d69 Compare May 2, 2022 16:16
@Kobzol
Copy link
Contributor Author

Kobzol commented May 2, 2022

Fixed tests and changed the assert to debug_assert.

@rylev rylev merged commit 203ac1a into master May 2, 2022
@rylev rylev deleted the pr-summary-raw-alignment branch May 2, 2022 18:24
nnethercote added a commit to nnethercote/rustc-perf that referenced this pull request Oct 28, 2022
The table spacing was made nice in rust-lang#1312 but then got messed up again
in rust-lang#1334.

This commit does the following.
- Fixes the table spacing.
- Changes how test failures are printed, so the expected and actual
  outputs are printed nicely across multiple lines, instead of being
  stringified with into a single line. I should have done this some time
  ago, it makes adjusting the expected output *so* much easier.
- Renames some variables, for clarity and consistency.
- Imports `std::cmp` because it's used in multiple places.
- Other minor tweaks.
nnethercote added a commit to nnethercote/rustc-perf that referenced this pull request Oct 28, 2022
The table spacing was made nice in rust-lang#1312 but then got messed up again
in rust-lang#1334.

This commit does the following.
- Fixes the table spacing.
- Changes how test failures are printed, so the expected and actual
  outputs are printed nicely across multiple lines, instead of being
  stringified with into a single line. I should have done this some time
  ago, it makes adjusting the expected output *so* much easier.
- Renames some variables, for clarity and consistency.
- Imports `std::cmp` because it's used in multiple places.
- Other minor tweaks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Would it be possible to make the perf results in markdown source more readable?
3 participants