pycobertura icon indicating copy to clipboard operation
pycobertura copied to clipboard

add possibility to specify the columns to show

Open gro1m opened this issue 2 years ago • 8 comments

Add possibility to hide columns.

Additional tests needed for the case where columns are hidden. There is a problem with two tests that files are not found, inexplicable to me, would be great if you could have a look at it @aconrad

gro1m avatar Oct 15 '22 15:10 gro1m

There is a problem with two tests that files are not found, inexplicable to me, would be great if you could have a look at it @aconrad

The two failing tests are:

  • test_diff__better_coverage_has_exit_status_of_zero
  • test_diff__worse_coverage_exit_status

Both tests pass the option --no-source which means that the source code shouldn't be rendered when generating the report, only the summary table. When --no-source is passed, it sets the flag DeltaReporter.show_source to False. When looking at the FileNotFound error, it's trying to open the file tests/dummy/__init__.py which indeed doesn't exist and yet, it is obviously a source file (.py). So for some reason, show_source is not honored.

On line 342 is where it fails while trying to open that file:

340  	class TextReporterDelta(DeltaReporter):
341  	    def generate(self):
342  ->	        lines = self.get_summary_lines()
343  	
344  	        if self.show_source:
345  	            missed_lines_colored = [
346  	                self.color_number(line) for line in lines["Missing"]
347  	            ]

~~... but self.show_source is checked after, on line 344~~ (EDIT: this is misleading. While true, it doesn't imply that get_summary_lines() shouldn't check for show_source).

So self.get_summary_lines() is trying to read the source file that doesn't exist and raises FileNotFound.

aconrad avatar Oct 16 '22 17:10 aconrad

Let me know if the explanation above helps. If not, I can take a closer look and see where exactly this is causing a problem.

aconrad avatar Oct 16 '22 17:10 aconrad

@aconrad Could you re-review? Certainly README update and CHANGELOG update are still needed, maybe also some more tests. What do you think?

gro1m avatar Oct 22 '22 22:10 gro1m

Another question popped up: Should we really make a hide columns feature or rather a show columns feature. The latter might allow then to specify a ordering of the columns like default or the order in which the columns are represented in the list or via an integer index. This idea came up due to this issue here: https://github.com/aconrad/pycobertura/issues/161. I do not think coloring a whole row is reasonable, but if we would decide to color the Cover column you might want to have that column as a first or second column.

gro1m avatar Oct 23 '22 09:10 gro1m

Should we really make a hide columns feature or rather a show columns feature. The latter might allow then to specify a ordering of the columns like default or the order in which the columns are represented in the list or via an integer index.

I generally tend to agree that "show" might be better than "hide" from a conceptual standpoint. It might also make the code more straightforward to read (dealing with "negatives" is usually trickier). I hadn't thought about the idea of using an array of columns to order the report's output, that could be useful.

If a "show" array of columns isn't provided (e.g. None), would you think of defaulting to the regular column output?

This idea came up due to this issue here: https://github.com/aconrad/pycobertura/issues/161. I do not think coloring a whole row is reasonable, but if we would decide to color the Cover column you might want to have that column as a first or second column.

I'm still on the fence about whether we should color the output of show, per my comment.

aconrad avatar Oct 25 '22 17:10 aconrad

Hi @aconrad What is the state of this PR? What I still realize is that the command line passing does not work:

pycobertura show coverage.xml --only-show-columns ["Filename"]
<...>
KeyError: '['

but

(py37) $ python -i
>>> import os
>>> import pytest
>>> from click.testing import CliRunner
>>> from pycobertura.cli import show, diff, ExitCodes
>>> runner = CliRunner()
>>> runner.invoke(show, ['tests/dummy.original.xml', '--only-show-columns',["Filename","Miss"]],catch_exceptions=False)
<Result okay>
>>> runner.invoke(show, ['tests/dummy.original.xml', '--only-show-columns',["Filename","Miss"]],catch_exceptions=False).output
'Filename             Miss\n-----------------  ------\ndummy/__init__.py       0\ndummy/dummy.py          2\nTOTAL                   2\n'

works. So the tests will be green, but on the command line it does not work. This is a bit frustrating. Do you have any input on this on what we can do here? Other that click may not support lists well and we might should pass it as a comma-separated string, which I initially proposed. Or we move to argparse, which is Python's native argument parser.

I mean there are some workarounds here, but they all seem a least a bit "hacky": https://stackoverflow.com/questions/47631914/how-to-pass-several-list-of-arguments-to-click-option.

gro1m avatar Feb 11 '23 11:02 gro1m

Hi @aconrad What is the state of this PR? What I still realize is that the command line passing does not work:

pycobertura show coverage.xml --only-show-columns ["Filename"]
<...>
KeyError: '['

but

(py37) $ python -i
>>> import os
>>> import pytest
>>> from click.testing import CliRunner
>>> from pycobertura.cli import show, diff, ExitCodes
>>> runner = CliRunner()
>>> runner.invoke(show, ['tests/dummy.original.xml', '--only-show-columns',["Filename","Miss"]],catch_exceptions=False)
<Result okay>
>>> runner.invoke(show, ['tests/dummy.original.xml', '--only-show-columns',["Filename","Miss"]],catch_exceptions=False).output
'Filename             Miss\n-----------------  ------\ndummy/__init__.py       0\ndummy/dummy.py          2\nTOTAL                   2\n'

works. So the tests will be green, but on the command line it does not work. This is a bit frustrating. Do you have any input on this on what we can do here? Other that click may not support lists well and we might should pass it as a comma-separated string, which I initially proposed. Or we move to argparse, which is Python's native argument parser.

I mean there are some workarounds here, but they all seem a least a bit "hacky": https://stackoverflow.com/questions/47631914/how-to-pass-several-list-of-arguments-to-click-option.

Hi @aconrad What is the state of this PR? What I still realize is that the command line passing does not work:

pycobertura show coverage.xml --only-show-columns ["Filename"]
<...>
KeyError: '['

but

(py37) $ python -i
>>> import os
>>> import pytest
>>> from click.testing import CliRunner
>>> from pycobertura.cli import show, diff, ExitCodes
>>> runner = CliRunner()
>>> runner.invoke(show, ['tests/dummy.original.xml', '--only-show-columns',["Filename","Miss"]],catch_exceptions=False)
<Result okay>
>>> runner.invoke(show, ['tests/dummy.original.xml', '--only-show-columns',["Filename","Miss"]],catch_exceptions=False).output
'Filename             Miss\n-----------------  ------\ndummy/__init__.py       0\ndummy/dummy.py          2\nTOTAL                   2\n'

works. So the tests will be green, but on the command line it does not work. This is a bit frustrating. Do you have any input on this on what we can do here? Other that click may not support lists well and we might should pass it as a comma-separated string, which I initially proposed. Or we move to argparse, which is Python's native argument parser.

I mean there are some workarounds here, but they all seem a least a bit "hacky": https://stackoverflow.com/questions/47631914/how-to-pass-several-list-of-arguments-to-click-option.

@aconrad Issues are resolved (have to admit used ChatGPT partly for this), so should be good to merge. The only thing to notice here, we have to quote the list when passing on the command line.

gro1m avatar Mar 25 '23 11:03 gro1m

@aconrad Want to bring your attention to this PR again, is there something I can do here to make it easier for yout to review: rebasing, reducing the number of changes?

gro1m avatar Apr 22 '23 10:04 gro1m