Skip to content
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

Split out the summary table into 2 tables #148

Merged
merged 2 commits into from
Oct 19, 2023
Merged

Conversation

zkoppert
Copy link
Member

@zkoppert zkoppert commented Oct 18, 2023

Addresses part of #144

Proposed Changes

Split out the summary table into 2 tables in order to improve readability

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request
  • run make lint and fix any issues that you have introduced
  • run make test and ensure you have test coverage for the lines you are introducing

Reviewer

  • Label as either bug, documentation, enhancement, infrastructure, or breaking

@zkoppert zkoppert added the enhancement New feature or request label Oct 18, 2023
@zkoppert zkoppert marked this pull request as ready for review October 18, 2023 21:35
@spier
Copy link
Contributor

spier commented Oct 18, 2023

If I were to set all of these configurations to TRUE, should the first table still be generated?
HIDE_TIME_TO_FIRST_RESPONSE, HIDE_TIME_TO_CLOSE, HIDE_TIME_TO_ANSWER

@spier
Copy link
Contributor

spier commented Oct 18, 2023

I confirmed it locally. If I set these configurations:

LABELS_TO_MEASURE = ""
HIDE_TIME_TO_FIRST_RESPONSE = TRUE
HIDE_TIME_TO_CLOSE = TRUE
HIDE_TIME_TO_ANSWER = TRUE

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:

| Metric | Average | Median | 90th percentile |
| --- | --- | --- | ---: |

rendered example:

Metric Average Median 90th percentile

Copy link
Contributor

@spier spier left a comment

Choose a reason for hiding this comment

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

Would be good to prevent possible generation of an empty table. See comment here: #148 (comment)

@@ -180,7 +180,7 @@ def write_overall_metrics_table(
columns,
file,
):
"""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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm on it! Thanks @spier!

Copy link
Member Author

Choose a reason for hiding this comment

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

Check out 3da633e

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@spier spier left a comment

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@zkoppert zkoppert merged commit 8fd4ca4 into main Oct 19, 2023
5 checks passed
@zkoppert zkoppert deleted the split-summary-table branch October 19, 2023 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants