Refactor: right align metrics email#1943
Conversation
a1270f2 to
fb68cfd
Compare
alanpeixinho
left a comment
There was a problem hiding this comment.
You are the G.O.A.T. Nice work
| diff = cur - prev | ||
| if diff == 0: | ||
| return "0 (0%)" | ||
| return "0" |
There was a problem hiding this comment.
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.
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.
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
| percentage = round((diff / prev) * 100) | ||
| percentage_sign = "+" if percentage > 0 else "" | ||
| return f"{signed_diff} ({percentage_sign}{percentage}%)" | ||
| return f"{signed_diff} ({percentage_sign}{percentage}%)" |
There was a problem hiding this comment.
nit: perhaps we could use f"{diff:+} {percentage:+}" here
There was a problem hiding this comment.
Sorry, I didn't get it. The difference between f"{signed_diff} ({percentage_sign}{percentage}%)" and f"{diff:+} {percentage:+}" would be from +100 (+200%) to +100 +200 for diff = 100 and prev = 50
There was a problem hiding this comment.
I only meant to use '+' format modifier, but is not necessary.
mentonin
left a comment
There was a problem hiding this comment.
Thanks for tackling this. I like that you included dynamic field spacing too! I left some comments for improvements, but they are not blockers
| {{ "{:>{width}}".format("Change", width=change_space) }} | ||
| {#-#} | ||
| {{ "{:<{width}}".format("", width=label_space) -}} | ||
| {{ "{:>{width}}".format("─" * (current_week_space-static_spacing), width=current_week_space) -}} |
There was a problem hiding this comment.
should we make this horizontal rule consistent with the other tables'?
| {{ "{:>{width}}".format(deltas.n_tests, width=change_space) }} | ||
|
|
||
|
|
||
| BUILD REGRESSIONS |
There was a problem hiding this comment.
BUILD REGRESSIONS should also be right aligned for consistency
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
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
There was a problem hiding this comment.
That works, done.
| {%- set lab_builds_space = [ns.lab_builds_len, "Covered builds"|length] | max + static_spacing -%} | ||
| {%- set lab_boots_space = [ns.lab_boots_len, "Boots"|length] | max + static_spacing -%} | ||
| {%- set lab_tests_space = [ns.lab_tests_len, "Tests"|length] | max + static_spacing -%} |
There was a problem hiding this comment.
You can drop these if you initialize ns with the label lengths instead of 0
| {%- set lab_change_len = deltas.labs.values() | map('length') | max -%} | ||
| {%- set lab_change_space = [lab_change_len, "Change (tests)"|length] | max + static_spacing -%} | ||
|
|
||
| {%- set separator_length = lab_names_space + lab_builds_space + lab_boots_space + lab_tests_space + lab_change_space - 2 -%} |
There was a problem hiding this comment.
nit: this is often called an horizontal rule (or rule, hrule, or hr) instead of the more generic "separator"
| {#- 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 ` *` -#} |
There was a problem hiding this comment.
nit: group all *_space for a table together
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
| diff = cur - prev | ||
| if diff == 0: | ||
| return "0 (0%)" | ||
| return "0" |
There was a problem hiding this comment.
Also, can we return a tuple of int instead? Then the template can align them separately, and we further separate data processing from templating
| {{ "{:>{width}}".format(fmt(0), width=lab_tests_space) -}} | ||
| {{ "{:>{width}}".format(deltas.labs.get(lab_key, ""), width=lab_change_space) }} | ||
| {% endfor %} | ||
| {{- " " + "─" * separator_length}} |
There was a problem hiding this comment.
formatting - can (should) we add jinja template formatting to the project?
There was a problem hiding this comment.
Unfortunately it's hard to get a good formatting here because if I just remove the " " it will be misaligned, if I add a space before {{"─" * separator_length}} it will contain the line break after {% endfor %}, and if I add the space there but add a -%} at the endfor to ignore the line break it will also ignore the space which will make it misaligned anyway.
I could of course keep it the way it was ({% endfor %} {{ "─" * separator_length}}) but IMO it looks worse having both commands in the same line
fb68cfd to
6ffa3e6
Compare
|
LGTM |
Readjusts the email formatting so that the larger number sections have a right-aligned text Part of kernelci#1938 Signed-off-by: Marcelo Robert Santos <4mrSantos@gmail.com>
Calculates the space required for the data and labels within jinja to avoid empty spaces or crammed values in the Coverage and Test Labs Activity sections It is possible to do it in Python but then we would need the fmt macro outside as well. A value such as 1000 might count as 4 chars in python but it is displayed as a 5-char 1,000 in jinja. Also, these spaces only make sense for the jinja template anyway, so I think it is correct to keep them within it. Signed-off-by: Marcelo Robert Santos <4mrSantos@gmail.com>
Closes kernelci#1938 Signed-off-by: Marcelo Robert Santos <4mrSantos@gmail.com>
6ffa3e6 to
381f98a
Compare
|
@mentonin I refactored the build regressions section to right align there too, but I'm using static spacing for now |
Readjusts the email formatting so that the larger number sections have a right-aligned text.
Changes
123 (+99%)into123 (+99%));(+0%)can clutter the change column.How to test
/backend/tests_submissions/submissions.zipfor testingpoetry run python3 manage.py notifications --action metrics_summaryand compare with the older versionVisual Reference
Example of new format:
Closes #1938