Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Local Compare mode HTML output files #1044

Merged
merged 1 commit into from
Nov 3, 2017
Merged

Local Compare mode HTML output files #1044

merged 1 commit into from
Nov 3, 2017

Conversation

csordasmarton
Copy link
Contributor

Local Compare mode should be able to generate HTML files with bug path.

Closes #748

@csordasmarton csordasmarton added enhancement 🌟 CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands labels Oct 17, 2017
@csordasmarton csordasmarton added this to the release 6.2 milestone Oct 17, 2017
@csordasmarton csordasmarton requested a review from gyorb October 17, 2017 11:05
gyorb
gyorb suggested changes Oct 17, 2017
View reviewed changes
Copy link
Contributor

@gyorb gyorb left a comment

Choose a reason for hiding this comment

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

Do we clean up the export output every time?
I think it can be misleading if we only add html reports to the directory. The run at the server might change and the local analysis reports can change too.

libcodechecker/cmd/cmd_line_client.py Outdated

output_path = os.path.join(args.export_dir,
filename + '_' + str(file_id) +
'_' + diff_type + '.html')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we should add the diff type to the filename.
I thought for each diff type I would create a separate export folder -e new_reports ...

libcodechecker/libclient/thrift_helper.py
@@ -105,6 +105,10 @@ def getRunData(self, run_name_filter):
pass

@ThriftClientCall
def getReportDetails(self, reportId):
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we miss this from the helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we did.

Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn't missed per-se. I think this method was never actually used in the command-line client ever before.

@csordasmarton
Copy link
Contributor Author

I have created a new clean argument option which cleans the output directory.

@csordasmarton csordasmarton requested a review from gyorb October 24, 2017 15:31
@csordasmarton csordasmarton force-pushed the diff_html branch 2 times, most recently from e8b8efd to dfdc7bf Compare October 24, 2017 16:20
@csordasmarton csordasmarton requested a review from bruntib October 25, 2017 07:40
whisperity
whisperity reviewed Oct 26, 2017
View reviewed changes
libcodechecker/cmd/cmd_line_client.py Outdated
report_to_html(client, reports, output_dir)

print('\nTo view the results in a browser run:\n'
'> firefox {0}'.format(args.export_dir))
Copy link
Contributor

Choose a reason for hiding this comment

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

What is >? Linux prompt character is $.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you print the directory path in this format file://home/myuser/report_directory it will be clickable in the terminal right?

Copy link
Contributor

Choose a reason for hiding this comment

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

In most Desktop terminal emulators, such as gnome-terminal, yes. I think the ancient xterm isn't capable of such feat.

whisperity
whisperity requested changes Oct 26, 2017
View reviewed changes
libcodechecker/cmd/cmd_line_client.py Outdated
print('Html file was generated for file {0}: {1}'.format(
checked_file, output_path))

def printReports(client, reports, output_format, diff_type):
Copy link
Contributor

Choose a reason for hiding this comment

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

Parts of this file contains functions that break this convention, but Python uses print_reports conventionally for function names. Let's not introduce more unneccessary breaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't break the python coding convention. The convention was broken in the #723 pull request. I created a separate issue to fix this (#1070).

gyorb
gyorb suggested changes Oct 30, 2017
View reviewed changes
Copy link
Contributor

@gyorb gyorb left a comment

Choose a reason for hiding this comment

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

Please add some description to the newly introduced methods, what they do and why do we need them.

libcodechecker/libhandlers/cmd.py
@@ -211,6 +228,20 @@ def __register_diff(parser):
help="Show results that appear in both the 'base' and "
"the 'new' run.")

def __handle(args):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you rename this to something more descriptive?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the usual name for this method in server and various other commands which have extra rules on their invocation.
This method only lives in the local scope of the registering function and isn't used anywhere else.

@csordasmarton csordasmarton requested a review from gyorb October 31, 2017 09:24
Xazax-hun
Xazax-hun suggested changes Oct 31, 2017
View reviewed changes
libcodechecker/cmd/cmd_line_client.py Outdated
@@ -321,7 +327,79 @@ def get_diff_report_dir(client, baseids, report_dir, diff_type):
filtered_reports.append(result)
return filtered_reports

def print_reports(client, reports, output_format):
def cache_report_file(file_cache, file_id):
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not like the name of this function. The main purpose is not to cache the file but to lookup a file based on a file id.

I would rename it to something like cached_report_file_lookup or get_report_file.

libcodechecker/cmd/cmd_line_client.py Outdated
file_report_map[report.fileId].append(report)

file_cache = {}
for file_id, file_reports in file_report_map.iteritems():
Copy link
Contributor

Choose a reason for hiding this comment

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

Is iteritems Python 3 compatible?

@gyorb gyorb removed this from the release 6.2 milestone Oct 31, 2017
Xazax-hun
Xazax-hun approved these changes Oct 31, 2017
View reviewed changes
Copy link
Contributor

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

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

LGTM!

@csordasmarton
Local Compare mode HTML output files
1d97c6f 
Local Compare mode should be able to generate HTML files with bug path.
@bruntib bruntib merged commit 71dee14 into Ericsson:master Nov 3, 2017
@csordasmarton csordasmarton deleted the diff_html branch November 6, 2017 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gyorb gyorb gyorb approved these changes

@Xazax-hun Xazax-hun Xazax-hun approved these changes

@bruntib bruntib Awaiting requested review from bruntib bruntib is a code owner

@whisperity whisperity Awaiting requested review from whisperity

Assignees
No one assigned
Labels
CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands enhancement 🌟
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@csordasmarton @Xazax-hun @whisperity @gyorb @bruntib