Skip to content

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

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

gdhameeja
Copy link
Contributor

@gdhameeja gdhameeja commented Mar 17, 2020

Closes #6906
code-highlight has default value to True, this value is used in TerminalWriter
in order to control highlighting of the code.

@gdhameeja gdhameeja changed the title Fix-6906: Added code-highlight option to disable highlighting optionally #6906: Added code-highlight option to disable highlighting optionally Mar 17, 2020
@gdhameeja gdhameeja changed the title #6906: Added code-highlight option to disable highlighting optionally Fix-6906: Added code-highlight option to disable highlighting optionally Mar 17, 2020
" {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}",
]
Copy link
Contributor Author

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.

@@ -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
Copy link
Contributor Author

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.

@gdhameeja
Copy link
Contributor Author

@nicoddemus review please?

Copy link
Member

@bluetech bluetech left a 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.

@blueyed
Copy link
Contributor

blueyed commented Mar 27, 2020

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)

@nicoddemus
Copy link
Member

It would be better to have a config setting that would allow for specifying the formatter/args for it also.

Agree, but out of scope for this PR.

@blueyed
Copy link
Contributor

blueyed commented Mar 28, 2020

It would be better to have a config setting that would allow for specifying the formatter/args for it also.

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.

@nicoddemus
Copy link
Member

An option to turn it on/off and selecting styles are separate options.

@blueyed
Copy link
Contributor

blueyed commented Mar 28, 2020

IMHO it would/could select the formatter, and "no" would use none, i.e. disable it then.
"auto" (which is used here, but unclear IIRC) would then e.g. use the best option (true colors, 256 colors, or 16 (what is used currently)).

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.

@blueyed

This comment has been minimized.

@nicoddemus
Copy link
Member

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.

@gdhameeja
Copy link
Contributor Author

@nicoddemus @bluetech fixed the comments. Can you please review it again. Thanks.

@The-Compiler
Copy link
Member

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?

Copy link
Member

@bluetech bluetech left a 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.

@bluetech
Copy link
Member

@The-Compiler

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?

Is there a general principle to commandline vs. ini? Looking at pytest --help I can't really discern one I must say ;)

@The-Compiler
Copy link
Member

Is there a general principle to commandline vs. ini? Looking at pytest --help I can't really discern one I must say ;)

Fair point. My personal take:

  • Something that someone would probably set for individual invocations rather than always (e.g. -x, -k, -m, --fixtures, --lf, ...): command-line option
  • Something that someone would probably want to always set (e.g. testpaths, xfail_strict, but also --strict): config option
  • Something requiring a lot of data (e.g. markers, filterwarnings): config option
  • Something where there could be multiple related options: config options

But I agree that's not really how things look at the moment, we also have lots of --doctest=* or --log-* options, so I don't have strong feelings on this.

Copy link
Member

@asottile asottile left a 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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this.

Copy link
Member

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?

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

@nicoddemus nicoddemus left a 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!

@gdhameeja gdhameeja force-pushed the Fix-6906 branch 3 times, most recently from dafe4f5 to 6455ece Compare June 15, 2020 19:00
@@ -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.

Copy link
Member

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

@@ -984,6 +984,7 @@ class ReprTraceback(TerminalRepr):
entrysep = "_ "

def toterminal(self, tw: TerminalWriter) -> None:
tw.code_highlight = False
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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.

@asottile @nicoddemus

Copy link
Member

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.

@@ -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
Copy link
Member

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?

Comment on lines 169 to 170
action="store",
dest="code_highlight",
Copy link
Member

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

ValueError,
match=re.escape("indents size (2) should have same size as lines (1)"),
):
tw._write_source(["assert 0"], [" ", " "])
Copy link
Member

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?

@gdhameeja gdhameeja force-pushed the Fix-6906 branch 2 times, most recently from 1483ea1 to 0ba5433 Compare June 15, 2020 19:42
@@ -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
Copy link
Member

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?

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured out the weirdness in that one test, I think this is good to go now

@nicoddemus
Copy link
Member

@bluetech would you like to take another look?

(Note to selves: squash on merge)

@bluetech
Copy link
Member

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. --code-highlight=<style>. If that doesn't look right, we should probably come up with something that would be...

@bluetech
Copy link
Member

@gnikonorov
Copy link
Member

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?

@nicoddemus
Copy link
Member

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. --code-highlight=<style>.

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. 👍

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Ran Benita <[email protected]>
@asottile asottile merged commit 61014c5 into pytest-dev:master Jun 26, 2020
@nicoddemus
Copy link
Member

Thanks @asottile for finishing it up. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to disable pygments highlighting even if pygments is installed
7 participants