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

[Fix #93] --html-detailed triggers html reports by itself #211

Merged
merged 1 commit into from Feb 12, 2018

Conversation

Projects
None yet
2 participants
@art-of-dom
Contributor

art-of-dom commented Feb 11, 2018

This fixes #93 with the main fix being a simple change to scripts/gcovr. The other changes were to remove the extra --html flags in the test makefiles to confirm the behaviour of the change.

Eventually, we should have separate tests for html and html-detailed, but that seemed like too big of a change for just this fix.

@latk

This comment has been minimized.

Member

latk commented Feb 11, 2018

This looks fantastic, thank you very much! I will merge this after I'm done with the gcovr-3.4 release, so in a few days.

Could you amend your commit to add your name to the AUTHORS.txt list? It is kept in alphabetical order, and you can use any name or handle you like. It is OK to update this PR by force-pushing to your branch.

As this is your first PR, I'm curious: How did you find out about this issue? Did you have any difficulty setting up a development environment for gcovr? Which documentation did you use? How can we make it easier to contribute? You don't have to answer, but an outside perspective would be very welcome. You can also contact me privately (my email can be found in the git log).

@art-of-dom

This comment has been minimized.

Contributor

art-of-dom commented Feb 11, 2018

Just about to force push the change now. The answers to your questions in order are:

  1. I've done work for the ceedling gcov plugin and instead of trying to home bake our own html report we've used gcovr. I've known about this for a bit, but didn't know that it was a quirk instead of a feature until I saw activity on issue #93.
  2. It wasn't too hard, but since this is a python project I would have preferred to have a requirements.txt for easy virtualenv use.
  3. I actually used the .travis.yml to figure out how to set up the environment and run the expected tests and linting since travis has to set up from scratch typically.
  4. The only thing I had an issue with is knowing what was expected in the PR. Specifically, I didn't know to update AUTHORS.txt and I didn't know how much testing update I should add to this. I've seen other projects use a template for both issues and pull requests to make it easier to convey to the person submitting each what should be included. That might help in this case.

@art-of-dom art-of-dom force-pushed the art-of-dom:html_detailed_flag_fix branch from 9205468 to d07d6f1 Feb 11, 2018

@latk

This comment has been minimized.

Member

latk commented Feb 11, 2018

Thank you, that was extremely helpful. I've added your suggestions to my todo-list and will try to implement them.

@latk

latk approved these changes Feb 12, 2018

@latk latk merged commit 877f732 into gcovr:master Feb 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment