[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] virsh: Enhance list command to ease creation of shell scripts

On 02/21/2012 06:05 PM, Eric Blake wrote:
On 02/21/2012 09:05 AM, Peter Krempa wrote:
  tools/virsh.c   |  176 ++++++++++++++++++++++++++++++++++++++++--------------
  tools/virsh.pod |   20 ++++++-
  2 files changed, 148 insertions(+), 48 deletions(-)

Thanks for picking up on my suggestions, and implementing it so quickly!

Well it makes it a lot easier to write shell scripts :)

+    if ((optTable&&  optName) ||
+        (optTable&&  optUUID) ||
+        (optName&&  optUUID)) {

I think it might be easier to write this as:

if (optTable + optName + optUUID>  1) {

That's elegant.

> -    if (desc) {
-        vshPrintExtra(ctl, "%-5s %-30s %-10s %s\n", _("Id"), _("Name"), _("State"), _("Title"));
-        vshPrintExtra(ctl, "-----------------------------------------------------------\n");
-    } else {
-        vshPrintExtra(ctl, " %-5s %-30s %s\n", _("Id"), _("Name"), _("State"));
-        vshPrintExtra(ctl, "----------------------------------------------------\n");

The old style printed a variable-length ---- line, depending on whether
title was in the mix...

It also broke the tests.

+    /* print table header in legacy mode */
+    if (optTable) {
+        vshPrintExtra(ctl, " %-5s %-30s %-10s",
+                      _("Id"), _("Name"), _("State"));
+        if (optTitle)
+            vshPrintExtra(ctl, "%-20s", _("Title"));
+        vshPrintExtra(ctl, "\n"
+                      "-----------------------------------------------------------\n");

but your new version prints a fixed-width ---- line as if title were
always present.  Not necessarily a show-stopper, but worth thinking about.

ACK with the virsh.pod typo fixes.

I fixed the typos and modified the default output to comply with the tests and pushed. Thanks.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]