Improve Terminal Output Formatting in pytest-cov - #678
Conversation
webknjaz
left a comment
There was a problem hiding this comment.
It'd be useful to see some output examples in the PR description, including comparison. Also, such changes usually require prior discussion.
|
Thanks for the feedback! My apologies for not initiating a discussion. I thought it'll be useful to have the code and output visible when discussing the change. Here's an example of the updated output (with a terminal width of 100) for clarity. Let me know if you have any concerns! Before After: |
|
I agree your proposed output looks nicer (and indeed it helps to have a "preview"!), just one nitpick about it: With do the two separators really make sense, given that there will only ever be one coverage report (I think?). I'd have expected something like: or maybe just |
|
As per suggestion, I moved the heading to It also helped me notice two small behaviors that I changed (I can revert them if needed):
|
|
current output (120 columns): |
Thank you :)
I saw in So, I assumed that using a constant Modifying or rearranging the |
|
Hey @The-Compiler , just checking in—do you have any further feedback on this? I’ve incorporated the suggested changes and would love to get this moving forward. Let me know if anything else needs adjusting! Also, I see there's a workflow approval pending—could someone with merge access take a look? |
The-Compiler
left a comment
There was a problem hiding this comment.
Two more small considerations. Other than that, this looks good to me, but I'll leave merging to someone who's more involved with pytest-cov than I am.
|
Thanks, @The-Compiler! I've addressed all the changes. I noticed the @maintainers link isn't working. Is there a way to ping someone for merging? |
|
As a general rule, please don't ping maintainers in their own projects unless it's something absolutely critical. You can expect maintainers to be subscribed to all activity in their own projects, and/or to be periodically checking things on their own schedule. Remember that projects are often maintained by a single person in their spare time, often with various other projects on their hands as well. You won't be the only person pinging them in a typical week, or sometimes even day. It usually make things far more stressful than they would be otherwise. Sometimes people are busy with other stuff, or on holidays, or have a tight work deadline, and then open source things will have to wait for a bit. |
|
Thank you. most of my coding was outside GitHub, so I am new to the GitHub system and etiquette, and happy to learn. |
|
That was fine IMHO, for two reasons:
|
|
Hey, just a quick check-in—I've addressed all requested changes, and the failing tests seem unrelated to this PR. Let me know if anything else is needed before merging. Thanks! |
|
While the failing tests may be unrelated, they may still block the PR from being merged. Sometimes, the fastest way of solving speeding up the process is making another PR fixing those problems and then rebasing. Most projects won't be force-merging PRs with red CI and would wish to keep the main branch green before anything is allowed in. The link check likely fails because GH changed the way they inject anchors into web pages. Sphinx has a toggle to skip this for certain websites. That would be the solution. But I'd also check that the linked thing still exists. |
|
You just have to wait patiently now and hope the primary maintainers get to it eventually. |
|
This seems fine, thank you. I'll try to release this week. |
This PR improves the formatting of separator lines in coverage reports and enhances the robustness of terminal width handling.
Why?
Changes:
-to_.