-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Improved the terminal colour formatting, Issue #11666 #12304
base: main
Are you sure you want to change the base?
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.
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.
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.
source, | ||
Lexer(), | ||
TerminalFormatter( | ||
# Establishes the style to be used. |
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 would omit these comments, instead let's try to make the code "speak for itself" :)
Lexer(), | ||
TerminalFormatter( | ||
# Establishes the style to be used. | ||
if os.environ.get("COLORTERM", "") not in ( |
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.
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.
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 |
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'd call this variable style
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.
Is there any prior art for those styles we can import
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
TerminalTrueColorFormatter
andTerminal256Formatter
to 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_THEME
is not set.Both
TerminalTrueColorFormatter
andTerminal256Formatter
do 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_MODE
sets if the light mode or dark mode default style is used. IfPYTEST_THEME_MODE
is 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