Skip to content
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

Add path context to filter options in config files #304

Closed
wants to merge 3 commits into from

Conversation

kjerstadius
Copy link
Contributor

@kjerstadius kjerstadius commented Apr 9, 2019

Addresses #300. From the "main" commit message:

By using a dedicated type for filter options it's possible include the
path context for a filter. This enables (relative) filters in config
files to be treated as relative to the location of the config file,
rather than relative to the current working directory.

Handling and behavior of CLI arguments is the same as before. Do note that the check_non_empty() function ended up being unused, so I removed it (in a separate commit though, so easy to roll back).

kjerstadius added 3 commits Apr 9, 2019
By using a dedicated type for filter options it's possible include the
path context for a filter. This enables (relative) filters in config
files to be treated as relative to the location of the config file,
rather than relative to the current working directory.
After the introduction of the FilterOption object this function is no
longer needed.
@@ -566,6 +567,7 @@ def merge_options_and_set_defaults(partial_namespaces, all_options=None):
help="Keep only gcov data files that match this filter. "
"Can be specified multiple times.",
action="append",
type=FilterOption,
Copy link
Contributor Author

@kjerstadius kjerstadius Apr 9, 2019

Choose a reason for hiding this comment

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

This option (and gcov_excludes as well) previously didn't use the check_non_empty() function. I wasn't sure if that was deliberate or not, but in the current iteration of the code that will be a slight behavioral change. I had to modify a test case to deal with this fact.

Copy link
Member

@latk latk Apr 10, 2019

Choose a reason for hiding this comment

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

I'll have to think a bit more about this.

An empty filter would be a regex that always matches, and can be used to deactivate a filter. This is most important for the --filter option to deactivate filtering (the default would filter to files under --root).

But I don't think this would be useful for --gcov-filter or --gcov-exclude: it doesn't make sense to exclude all files, and accepting all files already is the default. That only leaves use cases like “deactivating filters in a configuration file” which I'm not interested in supporting at the moment.

@@ -103,7 +103,7 @@ def test_line_threshold_100_1(capsys):
def test_filter_backslashes_are_detected(capsys):
c = capture(
capsys,
args=['--filter', r'C:\\foo\moo', '--gcov-exclude', ''],
Copy link
Contributor Author

@kjerstadius kjerstadius Apr 9, 2019

Choose a reason for hiding this comment

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

Couldn't really figure out the necessity of this argument as it seems to have no effect or relation at all to the actual test.

Copy link
Member

@latk latk May 26, 2019

Choose a reason for hiding this comment

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

I now understand why --gcov-exclude was used here. The capture() test driver runs gcovr's main(). It actually searches for coverage data and creates a report. But this test is not about the reports, only about side effects. Setting --gcov-exclude to an empty filter ensures that no coverage data will be found, which should make the test more robust. The empty filter is the easiest way to write a filter that always matches, here: excludes all files.

@codecov
Copy link

@codecov codecov bot commented Apr 9, 2019

Codecov Report

Merging #304 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
+ Coverage   94.89%   94.91%   +0.01%     
==========================================
  Files          15       15              
  Lines        1823     1830       +7     
  Branches      315      316       +1     
==========================================
+ Hits         1730     1737       +7     
  Misses         46       46              
  Partials       47       47
Impacted Files Coverage Δ
gcovr/utils.py 93.58% <100%> (+0.34%) ⬆️
gcovr/__main__.py 93.05% <100%> (ø) ⬆️
gcovr/tests/test_args.py 100% <100%> (ø) ⬆️
gcovr/configuration.py 97.2% <100%> (-0.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c018e2b...cd6461e. Read the comment docs.

class FilterOption(object):
def __init__(self, regex, path_context=None):
if not regex:
raise ArgumentTypeError("filter cannot be empty")
Copy link
Contributor Author

@kjerstadius kjerstadius Apr 9, 2019

Choose a reason for hiding this comment

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

Not sure if ArgumentTypeError is the most appropriate exception to use here, but I'd say it's the most straightforward to get behavior that's as similar as possible to before. I evaluated other alternatives but none really stuck. ValueError could be used, but using it will print a standardized error rather than a custom one. It will also print a traceback which the Argparse exception handling won't. If the behavior of the Argparse exception is to be preferred (mainly no traceback) a custom program exit function would need to be written. It would of course also be possible to leverage check_non_empty() to check that the value is set before passing it to the class constructor similar to how it was before, but then it would only apply to CLI args and not config file filters.

@latk latk closed this in 8ba5685 May 26, 2019
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.

None yet

2 participants