-
Notifications
You must be signed in to change notification settings - Fork 283
Add support for output directory #416
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 support for output directory #416
Conversation
c0d49a2
to
aee32a3
Compare
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.
Thank you so much for working on this frequently-requested feature. This looks pretty good, though I have some doubts about the data flow around check_output_file()
. It took me very long to convince myself that everything would work correctly also with values from config files.
Unfortunately I saved some comments before the last push, so they might already be marked as outdated 😅
e0ae8a0
to
8597789
Compare
Codecov Report
@@ Coverage Diff @@
## master #416 +/- ##
=========================================
Coverage ? 95.01%
=========================================
Files ? 20
Lines ? 2405
Branches ? 415
=========================================
Hits ? 2285
Misses ? 55
Partials ? 65
Continue to review full report at Codecov.
|
8597789
to
a372a9b
Compare
a372a9b
to
e91c4a3
Compare
e91c4a3
to
3d18542
Compare
3d18542
to
91c590e
Compare
Support for output directory as mentioned in gcovr#43 Fix test
91c590e
to
b658c36
Compare
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 think this is ready to be merged! I found the diff fairly hard to read, but that's partially due to having forgotten about the context of this PR, partially due to the not very explicit data model we are using.
E.g. I was wondering about whether filename.endswith(os.path.sep)
would also work as expected on Windows. Turns out yes because the paths are normalized during configuration/options handling. But the connection isn't obvious because the path is just a string. I think there is a future opportunity to clarify the data flows (also because we can now use type annotations), but definitely not as part of this PR.
Thanks for the work on this!
Support for output directory as mentioned in #43