Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.2k
gh-117225: Add color to doctest output#117583
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice! A few nits below -- it's a little hard to read the overall diff because of all the blank lines being added. Would you mind maybe leaving those out for now?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
I think we should also change the British spelling 'colour' to 'color' in the PR's title, which will become the commit message after merging. |
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
This reverts commitbb591b6.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
AlexWaygood commentedApr 6, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
What about also colorising the divider line that's used to separate failed examples? So it would look like this? It might help to see more clearly where one failed example ends and another begins. This patch does it: diff --git a/Lib/doctest.py b/Lib/doctest.pyindex 00ff1a107c..80d0b70592 100644--- a/Lib/doctest.py+++ b/Lib/doctest.py@@ -1307,7 +1307,10 @@ def report_unexpected_exception(self, out, test, example, exc_info): 'Exception raised:\n' + _indent(_exception_traceback(exc_info))) def _failure_header(self, test, example):- out = [self.DIVIDER]+ red, reset = (+ (ANSIColors.RED, ANSIColors.RESET) if can_colorize() else ("", "")+ )+ out = [f"{red}{self.DIVIDER}{reset}"] if test.filename: if test.lineno is not None and example.lineno is not None: lineno = test.lineno + example.lineno + 1@@ -1621,7 +1624,7 @@ def summarize(self, verbose=None): print(f" {green}{count:3d} test{s} in {name}{reset}") if failed:- print(self.DIVIDER)+ print(f"{red}{self.DIVIDER}{reset}") print(f"{red}{_n_items(failed)} had failures:{reset}") |
AlexWaygood commentedApr 6, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Of these three summary lines at the end:
You currently have all three colored in red if there are errors. I think I might be inclined to keep the last line colored in red (as you have it), butnot colorize the first two of those lines. I feel like having everything in red makes it harder to read, and having the last line in red is enough to draw your attention to it and communicate "bad!". What do you think? |
hugovk commentedApr 7, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I've added red divider lines. Edit: need to update tests to disable colour locally.
Can you share a screenshot and a diff to trigger it? I don't get all in red: |
AlexWaygood commentedApr 7, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Screenshot in#117583 (comment). For how to trigger it — I was testing by doing some silly things to some doctests in |
Uh oh!
There was an error while loading.Please reload this page.
AlexWaygood commentedApr 7, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
It looks like many tests fail if you run tests with Since there are so many that already fail with that environment variable set, I'm not sure it's really an issue, but thought I'd mention it anyway. |
…rom test_traceback to test__colorize
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks great to me! You might want to wait for a second review though (preferably from somebody who uses a light theme for their terminal 😄)
Privat33r-dev commentedApr 8, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I was pleased to see that The minor improvement that could be done is to use stub/other way to return empty colors so instead of this: ifcan_colorize():bold_green=ANSIColors.BOLD_GREENbold_red=ANSIColors.BOLD_REDgreen=ANSIColors.GREENred=ANSIColors.REDreset=ANSIColors.RESETyellow=ANSIColors.YELLOWelse:bold_green=""bold_red=""green=""red=""reset=""yellow="" it could be something like this: ANSIColors=get_ansi_colors()bold_green=ANSIColors.BOLD_GREENbold_red=ANSIColors.BOLD_REDgreen=ANSIColors.GREENred=ANSIColors.REDreset=ANSIColors.RESETyellow=ANSIColors.YELLOW |
Lib/_colorize.py Outdated
RED = "\x1b[31m" | ||
RESET = "\x1b[0m" | ||
YELLOW = "\x1b[33m" | ||
Privat33r-devApr 8, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ANSIColorsStub=type('ANSIColorsStub', (ANSIColors,), {x:""forxindir(ANSIColors)ifnotx.startswith('__')}) | |
defget_ansi_colors()->ANSIColors: | |
returnANSIColorsifcan_colorize()elseANSIColorsStub |
Usingget_ansi_colors()
preserves IDE suggestions (ctrl+space) and centralizes, as well as abstracts color availability logic, improving code's clarity and flexibility.
Uh oh!
There was an error while loading.Please reload this page.
Thanks for the suggestion, something like that sounds good. I'll wait to give#117672 a change to be merged first so as not to cause conflicts there, then will update this. I might include the refactoring as part of this PR, but mainly want to make sure this is merged before thebeta cutoff on 2024-05-07 so it can be in 3.13. I'll also add some docs, but they can be another PR and can be after the beta cutoff, if necessary. |
hugovk commentedApr 17, 2024 • edited by AlexWaygood
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by AlexWaygood
Uh oh!
There was an error while loading.Please reload this page.
Plan C: it's been a week and#117672 isn't merged yet. So in this PR, I've moved the colourise functionality back to the |
GREEN = "\x1b[32m" | ||
BOLD_GREEN = "\x1b[1;32m" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh, is there a reason why these colours might have been omitted from the original changes to colourise Python's tracebacks in general? Maybe to avoid accessibility issues for people who are red-green colourblind? Or to avoid issues for people who have light themes on their terminals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think because when there's a traceback error there's nothing "good" to report in green :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The patch still looks good to me, but I'd still love it if we could get a review from somebody who uses a light theme in their terminal and/or somebody who's colourblind before we merge 👍
The widely-used pytest (116m monthly downloads) uses the same ANSI colours: @encukou made a good point inpython/cherry-picker#120 (comment):
We're sticking to the 16 ANSI colours and have @pablogsal How does the following look for you? Is the text still legible? ![]() |
bedevere-bot commentedApr 24, 2024
|
Uh oh!
There was an error while loading.Please reload this page.
The doctest output looks like:
I moved the existing
_ANSIColors
class and_can_colorize
function fromtraceback.py
added in#112732, to re-use them, cc@pablogsal.Is
_colorize
an okay name for this module?If the module has leading underscore, do we also want leading underscores in
_ANSIColors
and_can_colorize
?