-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix-6906: Added code-highlight option to disable highlighting optionally #6934
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
Conversation
testing/test_terminal.py
Outdated
" {print}print{hl-reset}({str}'''{hl-reset}{str}{hl-reset}", | ||
"> {str} {hl-reset}{str}'''{hl-reset}); {kw}assert{hl-reset} {number}0{hl-reset}", | ||
"assert 0{reset}", | ||
] |
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 believe the test does not work, I'm not familiar with this format. I shall fix it.
testing/code/test_terminal_writer.py
Outdated
@@ -18,6 +18,7 @@ def test_code_highlight(has_markup, expected, color_mapping): | |||
f = StringIO() | |||
tw = TerminalWriter(f) | |||
tw.hasmarkup = has_markup | |||
tw.code_highlight = True |
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.
Added this with the assumption that this test assumes code is to be highlighted.
@nicoddemus review please? |
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.
Hi @gdhameeja,
I don't have time for a full review yet, but left some comments.
Also, the CI fails, which will need to be fixed before merging. You can click on the job in the "Checks" section and it will show you a log with what failed.
It would be better to have a config setting that would allow for specifying the formatter/args for it also. See #6658 for some feedback I've given there already. (it is rather bad that is uses the 16 colors formatter currently by default, and even goes through all the imports all the time) |
Agree, but out of scope for this PR. |
Well, you are about to add an option to control this behavior: it would be better to do it "properly" already then, and not add another option afterwards. |
An option to turn it on/off and selecting styles are separate options. |
IMHO it would/could select the formatter, and "no" would use none, i.e. disable it then. While having multiple options is not that bad I wanted to give my opinion anyway / still think it could be combined / should be discussed / considered. |
This comment has been minimized.
This comment has been minimized.
Not sure we will ever have a single option for color highlighting, we might grow some other options rather than just number of colors. I think having an explicit option to turn everything highlight related is more future proof. |
@nicoddemus @bluetech fixed the comments. Can you please review it again. Thanks. |
If we might want to have some more related options in the future, shouldn't this be an option in the .ini file rather than a commandline switch? |
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.
A couple of comments. Please also note that the CI checks fail.
Is there a general principle to commandline vs. ini? Looking at |
Fair point. My personal take:
But I agree that's not really how things look at the moment, we also have lots of |
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.
cool, looks good -- just needs a test to show that when the option is not "yes" that it is turned off -- also we'll need a changelog and probably some documentation as well
src/_pytest/config/__init__.py
Outdated
@@ -1260,6 +1260,7 @@ def create_terminal_writer(config: Config, *args, **kwargs) -> TerminalWriter: | |||
tw = TerminalWriter(*args, **kwargs) | |||
if config.option.color == "yes": | |||
tw.hasmarkup = True | |||
tw.code_highlight = False if config.option.code_highlight == "yes" else True |
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.
tw.code_highlight = False if config.option.code_highlight == "yes" else True | |
tw.code_highlight = config.option.code_highlight == "yes" |
(I have a wiki page about this)
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.
Fixed this.
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.
Still looks the same to me... did you forget to push it maybe?
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 like this didn't get fixed?
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.
Fixed.
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 @gdhameeja!
Besides the comments left by me and others, please also add:
- A CHANGELOG
- An entry in
reference.rst
describing this new option.
Let us know if you have any questions!
dafe4f5
to
6455ece
Compare
doc/en/changelog.rst
Outdated
@@ -504,6 +504,8 @@ Features | |||
such as default values or which set of command line options to add. | |||
|
|||
|
|||
- `#6906 <https://github.com/pytest-dev/pytest/issues/6906>`_: ``--code-highlight`` decides whethere code should be highlighted in terminal output. | |||
|
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.
we manage changelogs for new features by adding a changelog fragment. You can see how these are set up by looking at the ./changelog
directory (there's a bunch of files in there already that you can base yours on). The CONTRIBUTING guide also covers changelogs as well
src/_pytest/_code/code.py
Outdated
@@ -984,6 +984,7 @@ class ReprTraceback(TerminalRepr): | |||
entrysep = "_ " | |||
|
|||
def toterminal(self, tw: TerminalWriter) -> None: | |||
tw.code_highlight = False |
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.
was this left over from debugging? this doesn't quite feel right here
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.
No this was added purposely. The tests fail if code_highlight is set to true (which by default is it). I assumed all tests were written asserting to non highlighted code, so I put it here. Please guide me on this.
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 suspect this shouldn't happen here, and maybe we'll need to adjust some of the tests. I'm not 100% certain though
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.
FAILED testing/code/test_excinfo.py::TestFormattedExcinfo::test_repr_traceback_with_invalid_cwd - Failed: nomatch: 'def entry():'
FAILED testing/code/test_excinfo.py::TestFormattedExcinfo::test_exc_chain_repr_without_traceback[cause] - Failed: exact match: 'ValueError: invalid value'
FAILED testing/code/test_excinfo.py::TestFormattedExcinfo::test_exc_chain_repr_without_traceback[context] - Failed: exact match: 'ValueError: invalid value'
FAILED testing/io/test_terminalwriter.py::test_code_highlight[no markup] - AssertionError: assert ['\x1b[94mass...[39;49;00m\n'] == ['assert 0\n']
FAILED testing/acceptance_test.py::TestGeneralUsage::test_root_conftest_syntax_error - Failed: nomatch: '*raise SyntaxError*'
FAILED testing/acceptance_test.py::TestGeneralUsage::test_early_hook_error_issue38_1 - Failed: nomatch: '*INTERNALERROR*def pytest_sessionstart():*'
FAILED testing/acceptance_test.py::TestGeneralUsage::test_better_reporting_on_conftest_load_failure - assert ["ImportError...med 'qwerty'"] == ["ImportError...med 'qwerty'"]
FAILED testing/test_conftest.py::test_conftest_exception_handling - assert 'raise ValueError()' in ["ImportError while loading conftest '/tmp/pytest-of-melvault/pytest-0/test_conftest_exception_h...
FAILED testing/test_junitxml.py::TestPython::test_xfailure_xpass_strict[xunit1] - AssertionError: assert {'message': '...1B[39;49;00m'} == {'message': '...eds to fail!'}
FAILED testing/test_junitxml.py::TestPython::test_xfailure_xpass_strict[xunit2] - AssertionError: assert {'message': '...1B[39;49;00m'} == {'message': '...eds to fail!'}
FAILED testing/test_reports.py::TestReportSerialization::test_chained_exceptions[TestReport] - assert "ValueError('value error')" in "\x1b[94mdef\x1b[39;49;00m \x1b[92mtest_a\x1b[39;49;00m():\n ...
FAILED testing/test_reports.py::TestReportSerialization::test_chained_exceptions[CollectReport] - assert "ValueError('value error')" in "test_chained_exceptions.py:5: in test_a\n foo()\ntest_c...
FAILED testing/test_resultlog.py::TestWithFunctionIntegration::test_log_test_outcomes - assert -1 != -1
FAILED testing/python/collect.py::TestModule::test_show_traceback_import_error[0] - Failed: nomatch: 'ImportError while importing test module*'
FAILED testing/python/collect.py::TestModule::test_show_traceback_import_error[1] - Failed: nomatch: 'ImportError while importing test module*'
FAILED testing/python/collect.py::TestModule::test_show_traceback_import_error[2] - Failed: nomatch: 'ImportError while importing test module*'
FAILED testing/python/collect.py::TestModule::test_show_traceback_import_error_unicode - Failed: nomatch: 'ImportError while importing test module*'
These are all the tests which failed with code_highlight set to True
. I'm lost on what is the right way to do this. Please guide me here.
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.
Hi @gdhameeja,
The problem was the conditional in _highlight
: that should be an or
, not an and
. I've fixed it and pushed to your branch, hope you don't mind.
src/_pytest/config/__init__.py
Outdated
@@ -1260,6 +1260,7 @@ def create_terminal_writer(config: Config, *args, **kwargs) -> TerminalWriter: | |||
tw = TerminalWriter(*args, **kwargs) | |||
if config.option.color == "yes": | |||
tw.hasmarkup = True | |||
tw.code_highlight = False if config.option.code_highlight == "yes" else True |
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 like this didn't get fixed?
src/_pytest/terminal.py
Outdated
action="store", | ||
dest="code_highlight", |
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.
can leave these two lines out, they are the default when using argparse
testing/io/test_terminalwriter.py
Outdated
ValueError, | ||
match=re.escape("indents size (2) should have same size as lines (1)"), | ||
): | ||
tw._write_source(["assert 0"], [" ", " "]) |
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.
why is this missing now?
1483ea1
to
0ba5433
Compare
src/_pytest/config/__init__.py
Outdated
@@ -1260,6 +1260,7 @@ def create_terminal_writer(config: Config, *args, **kwargs) -> TerminalWriter: | |||
tw = TerminalWriter(*args, **kwargs) | |||
if config.option.color == "yes": | |||
tw.hasmarkup = True | |||
tw.code_highlight = False if config.option.code_highlight == "yes" else True |
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.
Still looks the same to me... did you forget to push it maybe?
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.
@bluetech would you like to take another look? (Note to selves: squash on merge) |
Also please see the discussion in #7132, where the ability to control the pygments style is discussed. If we want to support that, it would presumably use the same cmd line argument, e.g. |
There's also a |
Hi @gdhameeja. If this PR is meant to close an issue, could you please update the description as described here so that the linked issue may autoclose? |
I think we can postpone the discussion of styles to a separate PR, as I think it warrants more discussion ( #7132 (comment)) and either way whatever we decide later will probably be backward compatible. 👍 |
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.
Co-authored-by: Ran Benita <[email protected]>
Thanks @asottile for finishing it up. 👍 |
Closes #6906
code-highlight has default value to True, this value is used in
TerminalWriter
in order to control highlighting of the code.