Split out the summary table into 2 tables#148
Conversation
Signed-off-by: Zack Koppert <zkoppert@github.com>
|
If I were to set all of these configurations to TRUE, should the first table still be generated? |
|
I confirmed it locally. If I set these configurations: Then the 1st table would have no data other than the headline. Not if this would even render correctly in GFM. Testing here: resulting markdown code: rendered example:
|
spier
left a comment
spier
left a comment
There was a problem hiding this comment.
Would be good to prevent possible generation of an empty table. See comment here: #148 (comment)
| ): | ||
| """Write the overall metrics table to the markdown file.""" | ||
| """Write the overall metrics tables to the markdown file.""" | ||
| file.write("| Metric | Average | Median | 90th percentile |\n") |
There was a problem hiding this comment.
Maybe before doing any of this, one would have to check if the 1st stable will contain any data?
Something along the lines of this:
if "Time to first response" in columns \
or "Time to close" in columns \
or "Time to answer" in columns \
or labels:
There was a problem hiding this comment.
I did a quick test locally, and confirmed that this would indeed prevent the generation of the 1st table in this scenario.
I also confirmed that if at least one of these columns exists, the 1st table will be generated.
However we should probably add a real test case for this.
There was a problem hiding this comment.
Shifted the logic you suggested for the labels a bit since there is a corner case where there are labels but a user has marked HIDE_LABELS_COLUMN as True.
Signed-off-by: Zack Koppert <zkoppert@github.com>
spier
left a comment
spier
left a comment
There was a problem hiding this comment.
I didn't test this again now but given that you implemented a somewhat similar logic to what I tried previously, and you also added a test case, I am sure this is good :)
| "Time to first response" in columns | ||
| or "Time to close" in columns | ||
| or "Time to answer" in columns | ||
| or (hide_label_metrics is False and len(labels) > 0) |
There was a problem hiding this comment.
I don't fully get why this tests for "is False", even though the config variable can be "set to any value" to hide these metrics? I guess it is somehow tied to the "hide_label_metrics=False" in row 81 but I don't fully get that either :)
Btw is this way of handling ENV config variables a Python way of doing things? (I don't know Python that well)
There was a problem hiding this comment.
hide_label_metrics is False is another way of saying that the variable is not set. That is a python type thing from what I can tell.
2 participants
Addresses part of #144
Proposed Changes
Split out the summary table into 2 tables in order to improve readability
Readiness Checklist
Author/Contributor
make lintand fix any issues that you have introducedmake testand ensure you have test coverage for the lines you are introducingReviewer
bug,documentation,enhancement,infrastructure, orbreaking