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

[Carlo Caione]

On 12/02/2020 14:06, Jose E. Marchesi wrote:
> Hi Carlo!

Hey Jose,

/snip
> 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 
> uflags)
>      +{
>      +  /* set tree-print {no,deep,shallow}  */
>      +
>      +  const char *arg;
>      +
>      +  if (argc != 1)
>      +    assert (0);
>
> Why not assert (argc == 1)?

ACK

>      +
>      +  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 
> `shallow'.\n");
>
>
> 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"));

ACK

>      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,
>      +    PK_TREE_SHALLOW
>      +  };
>      +
>
> 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.

Right.

> It looks very good! :)

Thanks ;)

--
Carlo Caione