Improved the terminal colour formatting, Issue #11666 #12304
Conversation
added in the ability to use pygments TerminalTrueColorFormatter and Terminal256Formatter. I also created a default format which is similar to the default format in TerminalFormatter.
bluetech left a comment
bluetech left a comment There was a problem hiding this comment.
Thanks for working on this @FamousPond!
Here is my initial review:
Putting all of the code inside of _highlight makes it quite hard to follow. To factor it better, I made a change in #12443 to split it out to separate functions. Can you rebase your PR on the main branch? Then, you'd only really need to modify the _get_pygments_formatter function.
In pytest, the pygments library is optional, this means it may not be installed and importing it fails. So, we cannot define DarkModeStyle/LightModeStyle at the top level of terminalwriter.py. What I suggest is to define them in a new file, src/_pytest/_io/pygments_styles.py, and then import them lazily (i.e. not at the top level) in terminalwriter.py.
Regarding the DarkModeStyle and LightModeStyle, can you explain what you mean by "recreated to work with both TerminalTrueColorFormatter and Terminal256Formatter" means?
For me, the current (dark) color scheme looks like this:
With this PR, it looks like this:
I'm using Gnome Terminal with the following configuration:
I also left some inline comments.
src/_pytest/_io/terminalwriter.py Outdated
| source, | ||
| Lexer(), | ||
| TerminalFormatter( | ||
| # Establishes the style to be used. |
There was a problem hiding this comment.
I would omit these comments, instead let's try to make the code "speak for itself" :)
src/_pytest/_io/terminalwriter.py Outdated
| Lexer(), | ||
| TerminalFormatter( | ||
| # Establishes the style to be used. | ||
| if os.environ.get("COLORTERM", "") not in ( |
There was a problem hiding this comment.
This logic is repeated and split in several places. I suggest define a variable at the top, something like color_mode: Literal["basic", "256", "truecolor"], and use that.
src/_pytest/_io/terminalwriter.py Outdated
| if os.getenv("PYTEST_THEME") is None: | ||
| if os.getenv("PYTEST_THEME_MODE") is None: | ||
| # Neither PYTEST_THEME nor PYTEST_THEME_MODE have been set so using dark mode | ||
| SelectedStyle = DarkModeStyle |
RonnyPfannschmidt left a comment
RonnyPfannschmidt left a comment There was a problem hiding this comment.
Is there any prior art for those styles we can import
# Conflicts: # AUTHORS # src/_pytest/_io/terminalwriter.py
I was recreating the default light and dark mode formats that pygments uses. As seen here (although I was unsure what the '*' meant in Generic.Heading): Using the hardcoded classes may not be the best way to go about this, so if you have a better idea let me know. |
| @FamousPond the merge commit you pushed seemed to have removed most of your code changes, at least according to the PR's "Files changed". My recommendation is to use |
| @bluetech Sorry about that. I am still learning. Ill remember that for next time. I am a bit busy at the moment but haven't forgotten about this. I will try work on it next week. |
4 participants
Hello,
This PR resolves #11666.
This is my first time contributing to an open source project so please excuse any beginner mistakes.
I used the pull request #11700 as an initial guide as they started to implement this issue but then stopped.
I also know that there are errors with my pull request. I believe that they are all due to do the testing of the terminal output, which I have now changed. I am not sure how to change these tests properly, so have left it for now.
What I did
My goal was to implement the use of the pygment classes
TerminalTrueColorFormatterandTerminal256Formatterto get richer colours in the terminal .To do this I used the same logic as pygments use to get the formatter that is appropriate for the users computer.
https://github.com/pygments/pygments/blob/861fb9131b13241d7ea700fba8f6a38cf6f97285/pygments/cmdline.py#L448-L453C57
I set up the formatter in a way that the users can add their own style by setting the environmental variable
PYTEST_THEME. I also added in default styles which will be used ifPYTEST_THEMEis not set.Both
TerminalTrueColorFormatterandTerminal256Formatterdo not have light mode or dark mode options. I therefore created two default styles, one for each. I tried to create the styles as close as I could to the default style ofTerminalFormatter. The environmental variablePYTEST_THEME_MODEsets if the light mode or dark mode default style is used. IfPYTEST_THEME_MODEis not set dark mode is the default style.To create the default styles. I made classes at the end of the terminalwiter.py file. If this is the wrong spot for them I am sorry.
Thanks,
James