-
Notifications
You must be signed in to change notification settings - Fork 383
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
[plist2html] Adding severity to statistics page #2899
Conversation
@@ -220,6 +220,10 @@ def create_statistics_html(self, output_dir): | |||
Creates an statistics.html file which contains statistics information | |||
from the HTML generation process. | |||
""" | |||
def severity_order(severity): | |||
severities = ['CRITICAL', 'HIGH', 'MEDIUM', 'LOW', 'UNSPECIFIED'] |
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.
Please add a docstring and highlight that the order of the list members influences the sorting.
severity_statistics_content = '' | ||
for severity in sorted(severity_statistics, key=severity_order): | ||
num = severity_statistics[severity] | ||
severity_statistics_content += ''' |
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.
this will not be a long string right? StringIO could be used to build larger strings.
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.
If I execute plist-to-html binary (not the CodeChecker parse -o html command) I will get the following exception:
File "~/codechecker/tools/plist_to_html/plist_to_html/PlistToHtml.py", line 561, in main
html_builder.create_statistics_html(args.output_dir)
File "~/codechecker/tools/plist_to_html/plist_to_html/PlistToHtml.py", line 251, in create_statistics_html
'''.format(checker_name, self._severity_map[checker_name].lower(),
KeyError: 'bugprone-sizeof-expression'
for checker_name in sorted(checker_statistics): | ||
checker_statistics_content += ''' | ||
<tr> | ||
<td>{0}</td> | ||
<td class="severity" severity="{1}"> | ||
<i class="severity-{1}"></i> |
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.
It would be good to add a title
attribute to this element or for the td element so when the user hover the mouse over this column he will see what does the C severity means (critical).
severity_statistics_content += ''' | ||
<tr> | ||
<td class="severity" severity="{0}"> | ||
<i class="severity-{0}"></i> |
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.
Same as above (title attr).
@@ -220,6 +223,16 @@ def create_statistics_html(self, output_dir): | |||
Creates an statistics.html file which contains statistics information | |||
from the HTML generation process. | |||
""" | |||
def severity_order(severity): | |||
""" | |||
This function determines in whish order severities should be |
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.
This function determines in whish order severities should be | |
This function determines in which order severities should be |
|
||
substitute_data = self._tag_contents | ||
substitute_data.update({'table_reports': table_reports}) | ||
with io.StringIO() as table_reports: |
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.
Wow! Is this like using std::ostringstream
to buffer or format an output?
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.
Yes, it is.
<td>{0}</td> | ||
<td file="{3}" line="{4}"> | ||
<a href="{1}#reportHash={2}">{3} @ Line {4}</a> | ||
</td> | ||
<td class="severity" severity="{5}"> | ||
<i class="severity-{5}" title="{5}"></i> | ||
</td> | ||
<td>{6}</td> | ||
<td>{7}</td> | ||
<td class="bug-path-length">{8}</td> | ||
<td class="review-status review-status-{9}">{10}</td> | ||
</tr>'''.format(i + 1, | ||
os.path.basename(html_file), | ||
report['reportHash'], | ||
report['path'], | ||
events[-1]['location']['line'], | ||
severity.lower(), | ||
checker, | ||
events[-1]['message'], | ||
len(events), | ||
review_status.lower().replace(' ', '-'), | ||
review_status)) |
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.
There are waaaay too many placeholders here and it's getting confusing.
Could you turn this over to a dict
-based format
?
I've used that feature before, it looks like this: https://github.com/whisperity/envprobe/blob/f0eaf68bda67a0338af52d52026c83f8faaa61b8/shell/bash.py#L25-L59
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.
the new python3 f strings could be used too as a simplification
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.
Done
def severity_order(severity): | ||
""" | ||
This function determines in which order severities should be | ||
printed to the output. This function can be given via "key" | ||
attribute to sort() function. | ||
""" | ||
severities = ['CRITICAL', 'HIGH', 'MEDIUM', 'LOW', 'STYLE', | ||
'UNSPECIFIED'] | ||
return severities.index(severity) | ||
|
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 vaguely remember this exact enum existing somewhere in the core CodeChecker library... do we want to duplicate this, creating an additional maintenance point?
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.
the plist to html too is designed to be independent and self contained should not depend on any other parts
0a2f8c1
to
6da9fd0
Compare
In plist2html generator the severity is added to the static HTML statistics page.
@csordasmarton do you have any further comments? |
In plist2html generator the severity is added to the static HTML
statistics page.