aconrad / pycobertura

A code coverage diff tool for Cobertura reports
MIT License
114 stars 39 forks source link

add possibility to specify the columns to show #160

Open gro1m opened 1 year ago

gro1m commented 1 year ago

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

aconrad commented 1 year ago

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:

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 commented 1 year ago

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.

gro1m commented 1 year ago

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

gro1m commented 1 year ago

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.

aconrad commented 1 year ago

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.

gro1m commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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?