pr status: avoid printing a lonely "1" when there is only one Check#81
pr status: avoid printing a lonely "1" when there is only one Check#81
pr status: avoid printing a lonely "1" when there is only one Check#81Conversation
In a repository that only has a single Check configured (e.g. this repo), we would print "checks: 1" for PRs where the CI is passing. This looks akward when repeated for each PR and provides little useful information. This avoids ever printing "1" and instead prints "failing", "pending", or "success", respectively. We now only show numbers for repositories that have more than one Check runs.
|
I'm curious what @ampinsk thinks, but this makes me think we could show I find the floating 2 just as confusing as the floating 1; and subjectively I like knowing that either everything passed (and i don't care how many checks there are) or what number of them failed (preferably out of a total). |
That makes sense, let's get input from design and keep this conversation going. I would prefer to mimic the web UI and just show "success" or "failing" (or an icon, no numbers), but unlike in the web UI where you can click on that, the CLI right now wouldn't offer you any extra information about the failures. |
|
Hmm in my head, the states are:
Any thoughts on that? |
Now the possible outputs are: - "checks: pending" (yellow) - "checks: success" (green) - "checks: failing" (red) - 1 out of 1 check failed - "checks: 3/5 failing" (red) - 3 out of 5 checks failed
|
Thank you for the ideas! I've updated the behavior; the possible outputs are now:
I wanted to avoid showing "1/1" on repos that only have one check configured; let me know what you think about that please |
|
Thank you Mislav! Any thoughts on making them:
I also included some formatting tweaking here to align a little more with the designs and dotcom. Let me know what you think! |
|
@ampinsk done!
|
|
Yay looks great! Thanks Mislav! 🎉 |
probablycorey
left a comment
probablycorey
left a comment
There was a problem hiding this comment.
I've got one concern, but I don't want it to block shipping this.
| ratio = fmt.Sprintf("%d", checks.Total) | ||
| ratio = utils.Green(ratio) | ||
| summary = utils.Green("Checks passing") | ||
| } |
There was a problem hiding this comment.
I'm a little worried about this if block. It's hard to tell just from reading it if the summary will always be set.
There was a problem hiding this comment.
That's an interesting and valid point. I think that, for now, we can trust that the ChecksStatus object will either satisfy checks.Passing == checks.Total, or that either Failing or Pending will be non-zero. These are the only 3 possible scenarios and they're all covered. Of course, it might be possible that ChecksStatus() has a bug in it and return some inconsistent state, but if we discover that, we can fix it in CheckStatus and add more tests there. 👍
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
5 participants
In a repository that only has a single Check configured (e.g. this repo), we would print "checks: 1" for PRs where the CI is passing. This looks akward when repeated for each PR and provides little useful information.
This avoids ever printing "1" and instead prints "failing", "pending", or "success", respectively. We now only show numbers for PRs that have more than one Check run.