[Top][All Lists]

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

Re: [PATCH] cmd: set: Introduce tree-print set sub-command

From: Carlo Caione
Subject: Re: [PATCH] cmd: set: Introduce tree-print set sub-command
Date: Wed, 12 Feb 2020 14:38:47 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1

On 12/02/2020 14:06, Jose E. Marchesi wrote:

Hi Carlo!

Hey Jose,

What about:

  item_hdr=St_Item_Hdr {...}

That makes it more explicit that St_Item_Hdr is a struct, and also makes
it easier to glimpse that it can be further expanded.  WDYT?

No objections and it is indeed a nice improvement.

/snip> It would be nice to be able to define the maximum deep level, when
tree-print is "deep".  Something like:

(poke) .set tree-print-deep-level 3

Where the default coul be 0, meaning "no limit".

Also, it would be nice to be able to define the indentation step, in
number of spaces, like:

(poke) .set tree-print-indent 3

Where the default could be 3, like in your examples (which by the way
look very nice :)).

WDYT about adding these settings?

I can add those, no problem. I'm not a huge fan of having a separate set of set commands though (tree-print + tree-print-deep-level + tree-print-indent). I'll try to investigate if I can use a single tree-print set command with multiple sub-commands (i.e. `.set tree-print deep` + `.set tree-print deep-level 3` + `.set tree-print indent 3`).

     +static int
     +pk_cmd_set_tree_print (int argc, struct pk_cmd_arg argv[], uint64_t 
     +  /* set tree-print {no,deep,shallow}  */
     +  const char *arg;
     +  if (argc != 1)
     +    assert (0);

Why not assert (argc == 1)?


     +  arg = PK_CMD_ARG_STR (argv[0]);
     +  if (*arg == '\0')
     +    {
     +      enum pk_tree_mode tree_mode = pvm_tree_print (poke_vm);
     +      switch (tree_mode)
     +        {
     +        case PK_TREE_NO:      pk_puts ("no\n"); break;
     +        case PK_TREE_DEEP:    pk_puts ("deep\n"); break;
     +        case PK_TREE_SHALLOW: pk_puts ("shallow\n"); break;

PK_TREE_* seems to be very generic, and we may need it for something
else.  What about using, generally, another prefix such as PK_TREEP_*
for everything related to the tree printing?

Sounds good.

     +        default:
     +          assert (0);
     +        }
     +    }
     +  else
     +    {
     +      enum pk_tree_mode tree_mode;
     +      if (STREQ (arg, "no"))
     +        tree_mode = PK_TREE_NO;
     +      else if (STREQ (arg, "deep"))
     +        tree_mode = PK_TREE_DEEP;
     +      else if (STREQ (arg, "shallow"))
     +        tree_mode = PK_TREE_SHALLOW;
     +      else
     +        {
     +          pk_term_class ("error");
     +          pk_puts ("error: ");
     +          pk_term_end_class ("error");
     +          pk_puts (" tree_mode should be one of `no' or `deep' or 

I know I often forget to do so myself, but please use gettext to
internationalize strings for the user:

pk_puts (_(" tree_mode should be one of `no' or `deep' or `shallow'.\n"));


     diff --git a/src/pk-term.h b/src/pk-term.h
     index 80827a0b..8a20adcc 100644
     --- a/src/pk-term.h
     +++ b/src/pk-term.h
     @@ -23,6 +23,13 @@
#include <textstyle.h> +enum pk_tree_mode
     +  {
     +    PK_TREE_NO,
     +    PK_TREE_DEEP,
     +  };

Hm, I'm not sure this enumeration belongs to src/pk-term.h.  It is
the pvm subsystem that uses this, not services from pk-term.  What about
moving the definition there?

Yeah, I thought that in pvm.h that would have been a bit out-of-context but I was clearly wrong :)

      /* Initialize and finalize the terminal subsystem.  */
      void pk_term_init (int argc, char *argv[]);
      void pk_term_shutdown (void);
     @@ -40,6 +47,9 @@ extern void pk_puts (const char *str);
      /* Print a formatted string to the terminal.  */
      extern void pk_printf (const char *format, ...);
+/* Print a formatted string as indentation. */
     +extern void pk_printf_indent (unsigned int lvl);

I find the comment confusing.  This function is supposed to change the
indentation level of the output emitted by both pk_printf and pk_puts,
right?  Why including `printf' in the name?  Woulnd't it be better to
call it something like pk_term_indent?

Because originally that function was meant to do something different and I forgot to change the name (my original idea was to extend printf with a version printing also the indentation but then I changed my mind for "reasons" but the name stuck).

Also, we are gonna need a little update for the manual.


It looks very good! :)

Thanks ;)

Carlo Caione

reply via email to

[Prev in Thread] Current Thread [Next in Thread]