Refactor: right align metrics email #1943
+144
−90
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,12 +10,42 @@ Period: {{ start_datetime }} to {{ end_datetime }} | |
|
|
||
| COVERAGE | ||
| -------- | ||
| {%- set static_spacing = 2 -%} | ||
| {%- set current_week_space = [n_builds|string|length, n_boots|string|length, n_tests|string|length, "This week"|length] | max + static_spacing %} | ||
| {%- set previous_week_space = [prev_n_builds|string|length, prev_n_boots|string|length, prev_n_tests|string|length, "Last week"|length] | max + static_spacing %} | ||
| {%- set change_space = [deltas.n_builds|string|length, deltas.n_boots|string|length, deltas.n_tests|string|length, "Change"|length] | max + static_spacing -%} | ||
| {%- set label_space = 15 %} {#- length of the longest label -#} | ||
|
|
||
| {#- Each group is a row, each line is a column #} | ||
| {{ "{:<{width}}".format("", width=label_space) -}} | ||
| {{ "{:>{width}}".format("This week", width=current_week_space) -}} | ||
| {{ "{:>{width}}".format("Last week", width=previous_week_space) -}} | ||
| {{ "{:>{width}}".format("Change", width=change_space) }} | ||
| {#-#} | ||
| {{ "{:<{width}}".format("", width=label_space) -}} | ||
| {{ "{:>{width}}".format("─" * (current_week_space-static_spacing), width=current_week_space) -}} | ||
|
Contributor
There was a problem hiding this comment. should we make this horizontal rule consistent with the other tables'? |
||
| {{ "{:>{width}}".format("─" * (previous_week_space-static_spacing), width=previous_week_space) -}} | ||
| {{ "{:>{width}}".format("─" * (change_space-static_spacing), width=change_space) }} | ||
| {#-#} | ||
| {{"{:<{width}}".format("Trees monitored", width=label_space) -}} | ||
| {{ "{:>{width}}".format(fmt(n_trees), width=current_week_space) -}} | ||
| {{ "{:>{width}}".format(fmt(prev_n_trees), width=previous_week_space) -}} | ||
| {{ "{:>{width}}".format(deltas.n_trees, width=change_space) }} | ||
| {#-#} | ||
| {{ "{:<{width}}".format("Checkouts", width=label_space) -}} | ||
| {{ "{:>{width}}".format(fmt(n_checkouts), width=current_week_space) -}} | ||
| {{ "{:>{width}}".format(fmt(prev_n_checkouts), width=previous_week_space) -}} | ||
| {{ "{:>{width}}".format(deltas.n_checkouts, width=change_space) }} | ||
| {#-#} | ||
| {{ "{:<{width}}".format("Builds", width=label_space) -}} | ||
| {{ "{:>{width}}".format(fmt(n_builds), width=current_week_space) -}} | ||
| {{ "{:>{width}}".format(fmt(prev_n_builds), width=previous_week_space) -}} | ||
| {{ "{:>{width}}".format(deltas.n_builds, width=change_space) }} | ||
| {#-#} | ||
| {{ "{:<{width}}".format("Tests", width=label_space) -}} | ||
| {{ "{:>{width}}".format(fmt(n_tests), width=current_week_space) -}} | ||
| {{ "{:>{width}}".format(fmt(prev_n_tests), width=previous_week_space) -}} | ||
| {{ "{:>{width}}".format(deltas.n_tests, width=change_space) }} | ||
|
|
||
|
|
||
| BUILD REGRESSIONS | ||
|
Contributor
There was a problem hiding this comment. BUILD REGRESSIONS should also be right aligned for consistency
Contributor
Author
There was a problem hiding this comment. I think it's not needed to change build regressions since the scale there is smaller, eg. https://groups.io/g/kernelci-results/message/61332
Contributor
There was a problem hiding this comment. Maybe leave the auto-width out, but align the numbers right? the report you linked to still spans 3 orders of magnitude on Affected Builds
Contributor
Author
There was a problem hiding this comment. That works, done. |
||
|
|
@@ -24,30 +54,37 @@ Period: {{ start_datetime }} to {{ end_datetime }} | |
|
|
||
| {% if build_incidents_by_origin -%} | ||
|
|
||
| {% set origin_space = 12 -%} | ||
| {% set known_reg_space = 6 -%} | ||
| {% set new_reg_space = 6 -%} | ||
| {% set total_reg_space = 6 -%} | ||
| {% set reg_space = known_reg_space + new_reg_space + total_reg_space -%} | ||
| {% set affected_space = 16 -%} | ||
| {% set ind_issues_space = 10 -%} | ||
| {{ "{:<{width}}".format(" Origin", width=origin_space) -}} | ||
| {{ "{:>{width}}".format("Regressions", width=reg_space) -}} | ||
| {{ "{:>{width}}".format("Affected", width=affected_space) -}} | ||
| {{ "{:>{width}}".format("Affected builds by top issues", width=ind_issues_space*3 + 1) }} {#- +1 to get to a 2-space gap, same for the #1 issue and the space after total incidents #} | ||
| {{ "{:<{width}}".format("", width=origin_space) -}} | ||
| {{ "{:>{width}}".format("(known + new)", width=reg_space) -}} | ||
| {{ "{:>{width}}".format("Builds (total)", width=affected_space) -}} | ||
| {{ "{:>{width}}".format("#1", width=ind_issues_space + 1) -}}{{ "{:>{width}}".format("#2", width=ind_issues_space) -}}{{ "{:>{width}}".format("#3", width=ind_issues_space) }} | ||
| ─────────────────────────────────────────────────────────────────────────── | ||
| {% for origin, data in build_incidents_by_origin.items() | sort -%} | ||
| {{ "{:<{width}}".format(" " + origin, width=origin_space) -}} | ||
| {{ "{:>{width}}".format(fmt(data['n_existing_issues']) + " +", width=known_reg_space) }} | ||
| {{- "{:>{width}}".format(fmt(data['n_new_issues']) + " =", width=new_reg_space) }} | ||
| {{- "{:>{width}}".format(fmt(data['n_total_issues']), width=total_reg_space) -}} | ||
| {{ "{:>{width}}".format(fmt(data['total_incidents']), width=affected_space) -}} {{ " " -}} | ||
| {% for issue_key, data in top_issues_by_origin.get(origin, {}).items() -%} | ||
| {{ "{:>{width}}".format(fmt(data['total_incidents']), width=ind_issues_space) -}} | ||
| {% endfor %} | ||
| {% endfor %} ─────────────────────────────────────────────────────────────────────────── | ||
| {{ "{:<{width}}".format(" Total", width=origin_space) -}} | ||
| {{ "{:>{width}}".format(fmt(build_incidents_by_origin.values() | map(attribute='n_existing_issues') | sum) + " +", width=known_reg_space) }} | ||
| {{- "{:>{width}}".format(fmt(build_incidents_by_origin.values() | map(attribute='n_new_issues') | sum) + " =", width=new_reg_space) }} | ||
| {{- "{:>{width}}".format(fmt(build_incidents_by_origin.values() | map(attribute='n_total_issues') | sum), width=total_reg_space) -}} | ||
| {{ "{:>{width}}".format(fmt(build_incidents_by_origin.values() | map(attribute='total_incidents') | sum), width=affected_space) }} | ||
| {%- else %} No build regressions to show in this period. {%- endif %} | ||
|
|
||
|
|
||
|
|
@@ -80,34 +117,57 @@ Affected builds by top issues | |
|
|
||
| Labs marked with an asterisk (*) are new. | ||
|
|
||
| {% if n_labs and lab_maps -%} | ||
| {#- Compute the text spacing in jinja instead of python to consider the `fmt` macro -#} | ||
| {%- set static_spacing = 3 -%} | ||
| {%- set lab_names_len = lab_maps.keys() | map('length') | max -%} | ||
| {%- set lab_names_space = [lab_names_len, "Lab"|length ] | max + static_spacing + 2 -%} {#- +2 for the possible ` *` -#} | ||
|
Contributor
There was a problem hiding this comment. nit: group all *_space for a table together
Contributor
Author
There was a problem hiding this comment. Wdym by grouping? just remove the empty lines between the variables? I think it helps to discern which steps are happening and which order they'll appear |
||
|
|
||
| {#- a namespace is required to store the values outside of the loop -#} | ||
| {#- initialize with the label length so we consider them as well -#} | ||
| {%- set ns = namespace(lab_builds_len="Covered builds"|length, lab_boots_len="Boots"|length, lab_tests_len="Tests"|length) -%} | ||
| {%- for v in lab_maps.values() -%} | ||
| {%- set ns.lab_builds_len = [ns.lab_builds_len, (fmt(v.builds))|length] | max -%} | ||
| {%- set ns.lab_boots_len = [ns.lab_boots_len, (fmt(v.boots))|length] | max -%} | ||
| {%- set ns.lab_tests_len = [ns.lab_tests_len, (fmt(v.tests))|length] | max -%} | ||
| {%- endfor -%} | ||
| {%- set lab_builds_space = ns.lab_builds_len + static_spacing -%} | ||
| {%- set lab_boots_space = ns.lab_boots_len + static_spacing -%} | ||
| {%- set lab_tests_space = ns.lab_tests_len + static_spacing -%} | ||
|
|
||
| {%- set lab_change_len = deltas.labs.values() | map('length') | max -%} | ||
| {%- set lab_change_space = [lab_change_len, "Change (tests)"|length] | max + static_spacing -%} | ||
|
|
||
| {%- set hrule_length = lab_names_space + lab_builds_space + lab_boots_space + lab_tests_space + lab_change_space - 2 -%} | ||
|
|
||
| {{ "{:<{width}}".format(" Lab", width=lab_names_space) -}} | ||
| {{ "{:>{width}}".format("Covered builds", width=lab_builds_space) -}} | ||
| {{ "{:>{width}}".format("Boots", width=lab_boots_space) -}} | ||
| {{ "{:>{width}}".format("Tests", width=lab_tests_space) -}} | ||
| {{ "{:>{width}}".format("Change (tests)", width=lab_change_space) }} | ||
| {{ "─" * hrule_length}} | ||
| {% for lab_key, lab_values in lab_maps.items() | sort -%} | ||
| {%- set display_name = lab_key + " *" if lab_key in deltas.new_lab_keys else lab_key -%} | ||
| {{ "{:<{width}}".format(" " + display_name, width=lab_names_space) -}} | ||
| {{ "{:>{width}}".format(fmt(lab_values["builds"]), width=lab_builds_space) -}} | ||
| {{ "{:>{width}}".format(fmt(lab_values["boots"]), width=lab_boots_space) -}} | ||
| {{ "{:>{width}}".format(fmt(lab_values["tests"]), width=lab_tests_space) -}} | ||
| {{ "{:>{width}}".format(deltas.labs.get(lab_key, ""), width=lab_change_space) }} | ||
| {% endfor %} | ||
| {%- for lab_key in deltas.extinct_lab_keys | sort -%} | ||
| {%- set lab_values = prev_lab_maps[lab_key] -%} | ||
| {{ "{:<{width}}".format(" " + lab_key, width=lab_names_space) -}} | ||
| {{ "{:>{width}}".format(fmt(0), width=lab_builds_space) -}} | ||
| {{ "{:>{width}}".format(fmt(0), width=lab_boots_space) -}} | ||
| {{ "{:>{width}}".format(fmt(0), width=lab_tests_space) -}} | ||
| {{ "{:>{width}}".format(deltas.labs.get(lab_key, ""), width=lab_change_space) }} | ||
| {% endfor %} | ||
| {{- " " + "─" * hrule_length}} | ||
| {{ "{:<{width}}".format(" Total", width=lab_names_space) -}} | ||
| {{ "{:>{width}}".format(fmt(lab_maps.values() | map(attribute='builds') | sum), width=lab_builds_space) -}} | ||
| {{ "{:>{width}}".format(fmt(lab_maps.values() | map(attribute='boots') | sum), width=lab_boots_space) -}} | ||
| {{ "{:>{width}}".format(fmt(lab_maps.values() | map(attribute='tests') | sum), width=lab_tests_space) -}} | ||
| {{ "{:>{width}}".format(deltas.n_total_lab_activity, width=lab_change_space) }} | ||
|
|
||
| {%- endif %} | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question: is there any reason we are not just following the show_percentage parameter here? to decide if we should show or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, can we return a tuple of int instead? Then the template can align them separately, and we further separate data processing from templating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure tbh, but even still the lab changes are currently using percentages but they would be better off if the zeroes didn't have the percentage IMO