Add option --html-details-with-highlighting. #402
Conversation
Codecov Report
@@ Coverage Diff @@
## master #402 +/- ##
==========================================
+ Coverage 93.72% 93.93% +0.21%
==========================================
Files 20 20
Lines 2311 2342 +31
Branches 404 406 +2
==========================================
+ Hits 2166 2200 +34
+ Misses 77 75 -2
+ Partials 68 67 -1
Continue to review full report at Codecov.
|
0ec6c79 to
4754735
Compare
There was a problem hiding this comment.
Highlighting is a large quality of life improvement! I think the current implementation is a bit to complicated though:
- the new option should just modify the HTML generator, not add another way to add outputs
- highlighting should probably handle each file in one go, instead of highlighting line by line
I also have more general questions:
- Should highlighting be enabled by default? Pro: this is a quality of life improvement. Con: reports might be more difficult to read?
- I think a
Pygmentsdependency has to be added to the setup.py file. CI accidentally works, probably because Pygments is also installed for Sphinx. Or should Pygments be an optional dependency that we test for at runtime? - Is there any interaction between source code highlighting and the coloured backgrounds that indicate coverage (c.f. --html-theme)? Will highlighted code still be legible?
|
I've also a con:
|
|
The point that highlighting slows the report down is very good 🤔 The question is then what case should be optimized for: small to medium projects and users that are using gcovr for the first time, or larger projects. In one case there's non-obvious knowledge to make gcovr more useful, in the other non-obvious knowledge to make gcovr fast. I think that, within reason, the median project and the newer user should be prioritized over performance:
Perhaps a compromise to ensure discoverability is to time how long HTML report generation takes and write a Clippy-style suggestion to stderr when it takes too long? “Tip: you can speed up your HTML reports by disabling syntax highlighting with --no-html-details-highlighting”. In any case, that would be out of scope for this PR. |
30c95bc to
3a1dc63
Compare
|
The hilighting looks good in default and in blue theme. |
f890b32 to
13e398d
Compare
|
Is there at least one test for both options, with and without syntax highlighting? Just to ensure that both settings do not break in the future. |
|
Good point @chrta, I'll update one test to use |
While adding the tests I found out, that leading and trailing newlines where missing in the output of pygments. The default is to strip them. See option |
54a5fab to
f70c704
Compare
a677528 to
07f9d7d
Compare
| GcovrConfigOption( | ||
| "html_details_syntax_highlighting", ["--html-details-no-syntax-highlighting"], | ||
| group="output_options", | ||
| help="Disable syntax highlighting in HTML details page. " | ||
| "Default is active syntax highlighting.", | ||
| action="store_false", | ||
| ), |
There was a problem hiding this comment.
In the future, it's worth considering if we want to name negative options as --NAMESPACE-no-FEATURE or --no-NAMESPACE-FEATURE. I'm in the second camp because this allows positive/negative pairs to be autogenerated.
For example:
GcovrConfigOption(
"html_details_syntax_highlighting", ["--html-details-syntax-highlighting"],
group="output_options",
help="Use syntax highlighting in HTML details page. "
"Enabled by default.",
action="store_const",
default=True,
const=True,
const_negate=False, # autogenerates --no-NAME with action const=False
)Until now, the only use of this feature is the --html-self-contained/--no-html-self-contained option.
There are also concerns regarding configuration file support. A config key is derived for all options, unless explicitly disabled with config=False. It is not directly apparent what html-details-no-syntax-highlighting = yes would mean. Actually, I think there's a bug in the config file handling because config-key = no would reset to the default, which would make the above option impossible to disable. 🤔
There was a problem hiding this comment.
This is a Gold point. I haven't seen the existing negative Option. I also Luke the autogenerared - - no-... I'll Chance this in a separate PR.
Change option as mentioned in gcovr#402
Change option as mentioned in #402 Co-authored-by: Lukas Atkinson <opensource@LukasAtkinson.de>
Add optional syntax highlighting as added in #43.