8000 Template dir option by frankwiles · Pull Request #758 · gcovr/gcovr · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 44 commits into from
Aug 14, 2023
Merged

Template dir option #758

merged 44 commits into from
Aug 14, 2023

Conversation

frankwiles
Copy link
Contributor
@frankwiles frankwiles commented Mar 28, 2023

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

@vinniefalco
Copy link

This is quite frankly the most beautiful pull request I have ever seen in my life

@Spacetown Spacetown added this to the Upcoming release milestone Mar 29, 2023
Copy link
Member
@Spacetown Spacetown left a 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.

Spacetown and others added 9 commits April 4, 2023 16:08
* 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`
@frankwiles
Copy link
Contributor Author
frankwiles commented Apr 4, 2023

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.

@Spacetown
Copy link
Member

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.

@Spacetown
Copy link
Member

@frankwiles Please can you allow maintainers to push on your branch?

@frankwiles
Copy link
Contributor Author

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

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Honestly didn't know Github had that feature which is pretty neat.

@Spacetown
Copy link
Member
Spacetown commented Apr 7, 2023
8000

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

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Honestly didn't know Github had that feature which is pretty neat.

This is a checkbox when opening a PR. For already opened PRs it's on the right list at the end:
Bildschirm­foto 2023-04-07 um 20 47 28

@frankwiles
Copy link
Contributor Author

CleanShot 2023-04-07 at 14 04 08

Hmmm I'm not seeing an option there for that. Is this maybe because I forked this to an org rather than an individual?

@Spacetown
Copy link
Member

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.

@frankwiles
Copy link
Contributor Author

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! 😄

Copy link
Member
@Spacetown Spacetown left a 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.

@frankwiles
Copy link
Contributor Author

@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 html-css test and it's styles, etc. just overriding the contents of a <th> and the page title to show the template fall through is working.

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)

@Spacetown
Copy link
Member

The different files will be attached as Artifacts.

@frankwiles
Copy link
Contributor Author
frankwiles commented Jun 28, 2023

Ok, I'm starting to run out of ideas as to why this is happening. If I run pytest gcovr/tests/test_html_generator.py locally (removing the timeout option in setup.cfg as it seems to be missing in this version of pytest) the test passes just fine.

If I purposefully insert an assert False at the end you can see with these debugging prints it does as expected. Using the option works as expected when running gcovr itself, but when running nox it fails.

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?

CleanShot 2023-06-28 at 07 10 14@2x

Any insight or ideas to try would be appreciated @Spacetown

@Spacetown
Copy link
Member

Sorry, the next few weeks I can't check this. I'll investigate after my trip.

@frankwiles
Copy link
Contributor Author

@Spacetown no worries, thanks! Enjoy your trip!

@frankwiles
Copy link
Contributor Author

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

@Spacetown
Copy link
Member

I was 5 weeks on vacation and now I first want to fix the test on the Windows runners.

@frankwiles
Copy link
Contributor Author

That's great @Spacetown ! I'm jealous of so much vacation time! 😄

@frankwiles
Copy link
Contributor Author

Glad it wasn't just me who was baffled on that one! Ok, test removed and rebased with master.

@Spacetown
Copy link
Member

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

@frankwiles
Copy link
Contributor Author

@Spacetown sure thing, just sent you an invite to collaborate on it

@Spacetown
Copy link
Member

Pushing from my local machine still doesn't work.
In Codespaces or the conflict solver I can commit now.

@Spacetown
Copy link
Member

@frankwiles Anything to add or check from your side?

@frankwiles
Copy link
Contributor Author

No, everything looks good to me. Thanks for all your help on this @Spacetown !

Copy link
Member
@Spacetown Spacetown left a comment

Choose a reason for hiding this comment

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

LGFM

@Spacetown Spacetown merged commit 87634c1 into gcovr:master Aug 14, 2023
@Spacetown Spacetown removed this from the Upcoming release milestone Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Would you be open to a PR that adds in an option to use alternative templates?
3 participants
0