-
Notifications
You must be signed in to change notification settings - Fork 283
Template dir option #758
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
Template dir option #758
Conversation
Pass options down to `templates()` everywhere it's called. Change which jinja2 loader is used depending on existence of the argument.
This is quite frankly the most beautiful pull request I have ever seen in my life |
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.
Thanks for implementing this.
I'm restructuring the writers in a separate PR which will be merged today and you need to rebase.
Please add yourself to the CONTRIBUTORS.rst and the issue to the CHANGELOG.rst.
* Add thread to log messages if multithreading is active.
- Add comments and to collapse code and remove not needed endless loop. The loop is always executed only once because because the first statement in the block isn't changed in a second iteration. - Collapse also the data in the root. - Use `pop(key)` instead of copying the whole dict ignoring the value. - Fix evaluating of common path in nested HTML report. The root_filter wasn't applied to the root directory in HTML report since there is no trailing path separator at the end. Now the directories always have a trailing path separator which is removed in the report generation. To get the common path part the root directory is excluded. - Replace recursive generation of directory stats with a loop. - Use a object instead of a dataclass and remove the recursive function and the tests for it. - Improve detection of root directory.
* Move formats to sub directories. * Move function print_reports from __main__.py to writer/__init__.py. * Write global logger uppercase because it's constant. * Move gcov data handling to a own format handler * Use constants for the exit code. * Add prefix of format to each option. * Only forward the options of the format and some generic options to the handler. * Add one config key for each long option.
Pass options down to `templates()` everywhere it's called. Change which jinja2 loader is used depending on existence of the argument.
- Refactor based on suggestion
- Refactors code to use new `gcovr/formats/` layout - Renames option to `--html-template-dir` - Fixes merge artifacts I created on accident
This allows the user to only have to override the certain templates they wish to adjust and it will fall through to using the included ones for any template names the user has not overriden in `--html-template-dir`
I'm confused on this, I've rebased from upstream master and my local branch is clean of these conflicts. But the PR is still showing them. I did end up using VS Code merge tool (for the first time) so I suppose it's possible I messed something up but I have no conflict markers in these files. |
There was no merge from the master branch to yours. After merging there where two conflicts I need to resolve. |
@frankwiles Please can you allow maintainers to push on your branch? |
@Spacetown Sure, but I'm not finding where one does that in the UI. All I found was this page which says to do it on the PR list but I'm not seeing the option anywhere: Honestly didn't know Github had that feature which is pretty neat. |
8000
This is a checkbox when opening a PR. For already opened PRs it's on the right list at the end: |
That's possible. Please merge the main branch to your branch and solve the conflicts. There won't be a changed file but the merge information needs to be present. |
This is really weird, if I try to merge it I get THE EXACT same merge conflicts I got (and have resolved a couple of times now) rebasing. Which are the two files it says it has conflicts on. Any objections to me re-forking on my personal account, dropping in the changes and creating a new PR? I think this one is somehow cursed! 😄 |
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.
Some suggestions. After the merge from master to your branch there should be less changes to review.
@Spacetown sorry I got distracted by other things. I ran this locally and there are still some small differences in the HTML output, but the default templates haven't changed. I essentially copied the Hoping seeing the output in CI leads me to what else is going wrong as I can't run these tests locally (at least not meaningfully) |
The different files will be attached as Artifacts. |
Ok, I'm starting to run out of ideas as to why this is happening. If I run If I purposefully insert an Is it perhaps something related to the nox environment I'm not thinking of? Something that would perhaps cache or reuse something in a way I'm not realizing? Any insight or ideas to try would be appreciated @Spacetown |
Sorry, the next few weeks I can't check this. I'll investigate after my trip. |
@Spacetown no worries, thanks! Enjoy your trip! |
@Spacetown hi, just wanted to check in and see if you had a chance to look at this at all. So confused as to why it passes locally and doesn't in CI. |
I was 5 weeks on vacation and now I first want to fix the test on the Windows runners. |
That's great @Spacetown ! I'm jealous of so much vacation time! 😄 |
Co-authored-by: Michael Förderer <michael.foerderer@gmx.de>
Glad it wasn't just me who was baffled on that one! Ok, test removed and rebased with master. |
@frankwiles I want to finish this but without beeing able to push to the branch it's difficult. Can you please allow my account to push in general to your fork? |
@Spacetown sure thing, just sent you an invite to collaborate on it |
Pushing from my local machine still doesn't work. |
@frankwiles Anything to add or check from your side? |
No, everything looks good to me. Thanks for all your help on this @Spacetown ! |
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.
LGFM
This PR adds a
--template-dir
option to the CLI which allows the use of a directory of alternate Jinja2 templates for users who want to heavily customize their HTML output but don't want to fork gcovr to do so.I believe I've covered all the bases here, README, docs, etc but let me know if I've missed anything you would like to see.
Closes #714