LGTM, I noticed it only later when I reviewed the following patches.
On Mon, Feb 24, 2014 at 12:47 PM, Hrvoje Ribicic <[email protected]> wrote: > > > > On Fri, Feb 21, 2014 at 3:58 PM, Petr Pudlák <[email protected]> wrote: > >> >> >> >> On Tue, Feb 18, 2014 at 3:39 PM, Hrvoje Ribicic <[email protected]> wrote: >> >>> This patch changes gnt-job info to use standard functions defined in >>> cli.py, and output valid YAML. >>> >>> Signed-off-by: Hrvoje Ribicic <[email protected]> >>> --- >>> lib/client/gnt_job.py | 119 >>> ++++++++++++++++++++++---------------------------- >>> 1 file changed, 52 insertions(+), 67 deletions(-) >>> >>> diff --git a/lib/client/gnt_job.py b/lib/client/gnt_job.py >>> index 762ad51..d4cf2a7 100644 >>> --- a/lib/client/gnt_job.py >>> +++ b/lib/client/gnt_job.py >>> @@ -286,17 +286,6 @@ def ShowJobs(opts, args): >>> @return: the desired exit code >>> >>> """ >>> - def format_msg(level, text): >>> - """Display the text indented.""" >>> - ToStdout("%s%s", " " * level, text) >>> - >>> - def result_helper(value): >>> - """Format a result field in a nice way.""" >>> - if isinstance(value, (tuple, list)): >>> - return "[%s]" % utils.CommaJoin(value) >>> - else: >>> - return str(value) >>> - >>> selected_fields = [ >>> "id", "status", "ops", "opresult", "opstatus", "oplog", >>> "opstart", "opexec", "opend", "received_ts", "start_ts", "end_ts", >>> @@ -306,35 +295,32 @@ def ShowJobs(opts, args): >>> cl = GetClient() >>> result = cl.Query(constants.QR_JOB, selected_fields, qfilter).data >>> >>> - first = True >>> + job_info_container = [] >>> >>> for entry in result: >>> - if not first: >>> - format_msg(0, "") >>> - else: >>> - first = False >>> - >>> ((_, job_id), (rs_status, status), (_, ops), (_, opresult), (_, >>> opstatus), >>> (_, oplog), (_, opstart), (_, opexec), (_, opend), (_, recv_ts), >>> (_, start_ts), (_, end_ts)) = entry >>> >>> # Detect non-normal results >>> if rs_status != constants.RS_NORMAL: >>> - format_msg(0, "Job ID %s not found" % job_id) >>> + job_info_container.append("Job ID %s not found" % job_id) >>> continue >>> >>> - format_msg(0, "Job ID: %s" % job_id) >>> + # Container for produced data >>> + job_info = [("Job ID", job_id)] >>> + >>> if status in _USER_JOB_STATUS: >>> status = _USER_JOB_STATUS[status] >>> else: >>> raise errors.ProgrammerError("Unknown job status code '%s'" % >>> status) >>> >>> - format_msg(1, "Status: %s" % status) >>> + job_info.append(("Status", status)) >>> >>> if recv_ts is not None: >>> - format_msg(1, "Received: %s" % FormatTimestamp(recv_ts)) >>> + job_info.append(("Received", FormatTimestamp(recv_ts))) >>> else: >>> - format_msg(1, "Missing received timestamp (%s)" % str(recv_ts)) >>> + job_info.append(("Received", "unknown (%s)" % str(recv_ts))) >>> >>> if start_ts is not None: >>> if recv_ts is not None: >>> @@ -342,10 +328,10 @@ def ShowJobs(opts, args): >>> delta = " (delta %.6fs)" % d1 >>> else: >>> delta = "" >>> - format_msg(1, "Processing start: %s%s" % >>> - (FormatTimestamp(start_ts), delta)) >>> + job_info.append(("Processing start", "%s%s" % >>> + (FormatTimestamp(start_ts), delta))) >>> else: >>> - format_msg(1, "Processing start: unknown (%s)" % str(start_ts)) >>> + job_info.append(("Processing start", "unknown (%s)" % >>> str(start_ts))) >>> >>> if end_ts is not None: >>> if start_ts is not None: >>> @@ -353,64 +339,63 @@ def ShowJobs(opts, args): >>> delta = " (delta %.6fs)" % d2 >>> else: >>> delta = "" >>> - format_msg(1, "Processing end: %s%s" % >>> - (FormatTimestamp(end_ts), delta)) >>> + job_info.append(("Processing end", "%s%s" % >>> + (FormatTimestamp(end_ts), delta))) >>> else: >>> - format_msg(1, "Processing end: unknown (%s)" % str(end_ts)) >>> + job_info.append(("Processing end", "unknown (%s)" % str(end_ts))) >>> >>> if end_ts is not None and recv_ts is not None: >>> d3 = end_ts[0] - recv_ts[0] + (end_ts[1] - recv_ts[1]) / 1000000.0 >>> - format_msg(1, "Total processing time: %.6f seconds" % d3) >>> + job_info.append(("Total processing time", "%.6f seconds" % d3)) >>> else: >>> - format_msg(1, "Total processing time: N/A") >>> - format_msg(1, "Opcodes:") >>> + job_info.append(("Total processing time", "N/A")) >>> + >>> + opcode_container = [] >>> for (opcode, result, status, log, s_ts, x_ts, e_ts) in \ >>> zip(ops, opresult, opstatus, oplog, opstart, opexec, opend): >>> - format_msg(2, "%s" % opcode["OP_ID"]) >>> - format_msg(3, "Status: %s" % status) >>> + opcode_info = [] >>> + opcode_info.append(("Opcode", opcode["OP_ID"])) >>> + opcode_info.append(("Status", status)) >>> + >>> if isinstance(s_ts, (tuple, list)): >>> - format_msg(3, "Processing start: %s" % FormatTimestamp(s_ts)) >>> + opcode_info.append(("Processing start", FormatTimestamp(s_ts))) >>> else: >>> - format_msg(3, "No processing start time") >>> + opcode_info.append(("Processing start", "N/A")) >>> >> >> This looks like a recurring piece of code in the following few lines, so >> perhaps we could further improve it by adding an inner function that'd just >> take the name of the key and a something_ts value. >> >> > This is addressed in the following patches, as I wanted to both limit the > number of changes in one patch, and add alignment support. > > >> + >>> if isinstance(x_ts, (tuple, list)): >>> - format_msg(3, "Execution start: %s" % FormatTimestamp(x_ts)) >>> + opcode_info.append(("Execution start", FormatTimestamp(x_ts))) >>> else: >>> - format_msg(3, "No execution start time") >>> + opcode_info.append(("Execution start", "N/A")) >>> + >>> if isinstance(e_ts, (tuple, list)): >>> - format_msg(3, "Processing end: %s" % FormatTimestamp(e_ts)) >>> + opcode_info.append(("Processing end", FormatTimestamp(e_ts))) >>> else: >>> - format_msg(3, "No processing end time") >>> - format_msg(3, "Input fields:") >>> - for key in utils.NiceSort(opcode.keys()): >>> - if key == "OP_ID": >>> - continue >>> - val = opcode[key] >>> - if isinstance(val, (tuple, list)): >>> - val = ",".join([str(item) for item in val]) >>> - format_msg(4, "%s: %s" % (key, val)) >>> - if result is None: >>> - format_msg(3, "No output data") >>> - elif isinstance(result, (tuple, list)): >>> - if not result: >>> - format_msg(3, "Result: empty sequence") >>> - else: >>> - format_msg(3, "Result:") >>> - for elem in result: >>> - format_msg(4, result_helper(elem)) >>> - elif isinstance(result, dict): >>> - if not result: >>> - format_msg(3, "Result: empty dictionary") >>> - else: >>> - format_msg(3, "Result:") >>> - for key, val in result.iteritems(): >>> - format_msg(4, "%s: %s" % (key, result_helper(val))) >>> - else: >>> - format_msg(3, "Result: %s" % result) >>> - format_msg(3, "Execution log:") >>> + opcode_info.append(("Processing end", "N/A")) >>> + >>> + opcode_info.append(("Input fields", opcode)) >>> + opcode_info.append(("Result", result)) >>> + >>> + exec_log_container = [] >>> for serial, log_ts, log_type, log_msg in log: >>> time_txt = FormatTimestamp(log_ts) >>> encoded = FormatLogMessage(log_type, log_msg) >>> - format_msg(4, "%s:%s:%s %s" % (serial, time_txt, log_type, >>> encoded)) >>> + >>> + # Arranged in this curious way to preserve the brevity for >>> multiple >>> + # logs. This content cannot be exposed as a 4-tuple, as time >>> contains >>> + # the colon, causing some YAML parsers to fail. >>> + exec_log_info = [("Time", time_txt), >>> + ("Content", (serial, log_type, encoded,)), >>> + ] >>> + exec_log_container.append(exec_log_info) >>> + opcode_info.append(("Execution log", exec_log_container)) >>> + >>> + opcode_container.append(opcode_info) >>> + >>> + job_info.append(("Opcodes", opcode_container)) >>> + job_info_container.append(job_info) >>> + >>> + PrintGenericInfo(job_info_container) >>> + >>> return 0 >>> >>> >>> -- >>> 1.9.0.rc1.175.g0b1dcb5 >>> >>> >> Rest LGTM. >> >> Note: This is a great improvement. I just found one small "regression", >> and that is that result keys were NiceSorted before, but now they're sorted >> just lexicographically. I don't know if it's worth doing something about >> that, perhaps to sort all dictionary keys in _SerializeGenericInfo using >> NiceSort, or just keep it as it is (both cases LGTM). >> >> >
