hierarchy-print doesn't allow sending indent-string arg to hierarchy-map

hierarchy-print doesn't allow sending indent-string arg to hierarchy-map

[Damien Cassou]

Hi,

you are right that something isn't good enough. The fix you suggest
fixes the problem. Unfortunately, it is backward incompatible: all
usages of `hierarchy-print' passing a custom value for TO-STRING will
now fail because TO-STRING should now read a second argument.
Additionally, I think it shouldn't be the responsibility of
`hierarchy-print' to take care of the indentation.

Had I been smart enough, I would probably have written `hierarchy-print'
this way instead:

  (defun hierarchy-print (hierarchy &optional to-string)
    (let ((to-string (or to-string (lambda (item) (format "%s" item)))))
      (hierarchy-map
       (lambda (item indent)
         (insert (funcall to-string item ident) "\n"))
       hierarchy)))

That is:
- TO-STRING takes 2 parameters

- indentation should be decided by the user through a dedicated value
  TO-STRING value, e.g., with
  (hierarchy-labelfn-indent (lambda (item) (format "%s" item)))

But this implementation also breaks backward compatibility. What I
suggest:

1. Implement `hierarchy-print-line' with the implementation above (this
   way we have a good implementation anyone can use)

2. Refactor and simplify `hierarchy-print` to avoid duplication with
   `hierarchy-print-line' but keeping backward compatibility.

What do you think? Would you mind sending a patch?

(please keep me on CC)

Best

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

Re: hierarchy-print doesn't allow sending indent-string arg to hierarchy-map

[mousebot]

Hi Damien,

I tried our your suggestion in my library, and it works fine.

My initial query was also a way of asking if there is any (other) way in hierarchy.el of printing while respecting indentation. I don't know the library well at all. I guess the answer is no, given your response?

I'm unsure as to how to refactor hierarchy-print, as I don't understand what you think it ought to do differently once hierarchy-print-line is added.

I also wonder if it's perhaps a little confusing to have an arg 'to-string' that is now likely to be a call to `hierarchy-labelfn-indent`, whereas in `hierarchy-print` `to-string` is a function handed to hierarchy-labelfn-indent as an argument.

In that case, perhaps -print-line should have the same call to `hierarchy-labelfn-indent` in its body, so that the to-string arg is of a similar form to -print?

Finally, is there anything wrong with simply making the indent arg in the lambda in my original suggestion '&optional'?

i.e.

(defun hierarchy-print (hierarchy &optional to-string indent-string)
   (let ((to-string (or to-string (lambda (item) (format "%s" item)))))
     (hierarchy-map
      (hierarchy-labelfn-indent (lambda (item &optional indent)
                                  (insert (funcall to-string item indent) "\n"))
                                indent-string)
      hierarchy)))

Maybe sending 'nil' as an argument to funcall is bad news?

I don't really understand why the -print function shouldn't take an indent-string argument, but it's your pkg not mine.

kind regards,
m


On 7/26/23 18:16, Damien Cassou wrote:
> Hi,
> 
> you are right that something isn't good enough. The fix you suggest
> fixes the problem. Unfortunately, it is backward incompatible: all
> usages of `hierarchy-print' passing a custom value for TO-STRING will
> now fail because TO-STRING should now read a second argument.
> Additionally, I think it shouldn't be the responsibility of
> `hierarchy-print' to take care of the indentation.
> 
> Had I been smart enough, I would probably have written `hierarchy-print'
> this way instead:
> 
>    (defun hierarchy-print (hierarchy &optional to-string)
>      (let ((to-string (or to-string (lambda (item) (format "%s" item)))))
>        (hierarchy-map
>         (lambda (item indent)
>           (insert (funcall to-string item ident) "\n"))
>         hierarchy)))
> 
> That is:
> - TO-STRING takes 2 parameters
> 
> - indentation should be decided by the user through a dedicated value
>    TO-STRING value, e.g., with
>    (hierarchy-labelfn-indent (lambda (item) (format "%s" item)))
> 
> But this implementation also breaks backward compatibility. What I
> suggest:
> 
> 1. Implement `hierarchy-print-line' with the implementation above (this
>     way we have a good implementation anyone can use)
> 
> 2. Refactor and simplify `hierarchy-print` to avoid duplication with
>     `hierarchy-print-line' but keeping backward compatibility.
> 
> What do you think? Would you mind sending a patch?
> 
> (please keep me on CC)
> 
> Best
> 

Re: hierarchy-print doesn't allow sending indent-string arg to hierarchy-map

[Damien Cassou]

Hi,

mousebot <mousebot@riseup.net> writes:
> My initial query was also a way of asking if there is any (other) way
> in hierarchy.el of printing while respecting indentation.

I think `hierarchy-print' is what you need. As you can see, the
implementation is very short so you can also copy/paste/adapt the code
of the function to your needs.


> I'm unsure as to how to refactor hierarchy-print, as I don't
> understand what you think it ought to do differently once
> hierarchy-print-line is added.

The goal is to have hierarchy-print keep its current behavior but
leverage `hierarchy-print-line' for its implementation. I have something
like this in mind (not tested):

(defun hierarchy-print (hierarchy &optional to-string)
  "Insert HIERARCHY in current buffer as plain text.

Use TO-STRING to convert each element to a string.  TO-STRING is
a function taking an item of HIERARCHY as input and returning a
string.  If nil, TO-STRING defaults to a call to `format' with \"%s\"."
  (hierarchy-print-line
    hierarchy
    (hierarchy-labelfn-indent
      (lambda (item _) (insert (funcall to-string item) "\n")))))

> I also wonder if it's perhaps a little confusing to have an arg
> 'to-string' that is now likely to be a call to
> `hierarchy-labelfn-indent`, whereas in `hierarchy-print` `to-string`
> is a function handed to hierarchy-labelfn-indent as an argument.


hierarchy has a notion of "labeling function" (parameters of this type
are called LABELFN). A labeling function is one that takes an item
object and an indentation level number as arguments and returns a
string. The `hierarchy-labelfn-indent' function takes a "labeling
function" as argument and returns another one.

The function that `hierarchy-print' takes as parameter is called
TO-STRING instead of LABELFN because it only accepts an item object as
argument.


> In that case, perhaps -print-line should have the same call to
> `hierarchy-labelfn-indent` in its body, so that the to-string arg is
> of a similar form to -print?


I disagree. In `hierarchy-print-line', the second parameter should be
named LABELFN instead of TO-STRING (sorry for not saying that
earlier). The docstring should say that the function is not responsible
for indentation and that the user is free to call
`hierarchy-labelfn-indent' or `hierarchy-print' if s/he cares about
indentation.


> Finally, is there anything wrong with simply making the indent arg in
> the lambda in my original suggestion '&optional'?
>
> i.e.
>
> (defun hierarchy-print (hierarchy &optional to-string indent-string)
>    (let ((to-string (or to-string (lambda (item) (format "%s" item)))))
>      (hierarchy-map
>       (hierarchy-labelfn-indent (lambda (item &optional indent)
>                                   (insert (funcall to-string item indent) "\n"))
>                                 indent-string)
>       hierarchy)))

The problem is not this lambda as `hierarchy-labelfn-indent' will always
call it with 2 arguments anyway. The problem is with the TO-STRING
parameter which used to be called with 1 argument but now requires
2. For example, the code below works before the change you suggest but
triggers an error after:

  (hierarchy-print hierarchy (lambda (item) (format "-%s-")))

This is because the lambda here will now be called with 2 arguments but
it only declares 1 parameter.

To go in the direction you suggest, we could use `func-arity' on
TO-STRING to detect how many arguments we have to pass and then call it
with either both ITEM and INDENT or only with ITEM.

> Maybe sending 'nil' as an argument to funcall is bad news?


it's not the problem


> I don't really understand why the -print function shouldn't take an
> indent-string argument, but it's your pkg not mine.


this is not the problem either. The problem is passing this argument to
TO-STRING. I hope the above would have make this clearer.

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

Re: hierarchy-print doesn't allow sending indent-string arg to hierarchy-map

[mousebot]

[-- Attachment #1: Type: text/plain, Size: 5130 bytes --]

hi again,

thanks for clarifying, i was confused by to-string/labelfn args.

i came up with something based on what you suggested. with this the code in my pkg works fine, and the hierarchy.el tests all pass.

to fix the tests for 'hierarchy-print', i found i had to also remove 'insert' inside the lambda that is the LABELFN arg for 'hierarchy-labelfn-indent', as insert returns nil and we need to return a string. hierarchy-print-line how handles 'insert'. i'm unsure about whether the let binding of to-string in -print is needed, and whether it should be the same as the let binding of labelfn in -print-line.

perhaps there is some other code it run it against also?

a patch is attached, formatted with magit from a magit diff. is that applicable, or is a different format required?

kind regards,
m


On 7/30/23 08:16, Damien Cassou wrote:
> Hi,
> 
> mousebot <mousebot@riseup.net> writes:
>> My initial query was also a way of asking if there is any (other) way
>> in hierarchy.el of printing while respecting indentation.
> 
> I think `hierarchy-print' is what you need. As you can see, the
> implementation is very short so you can also copy/paste/adapt the code
> of the function to your needs.
> 
> 
>> I'm unsure as to how to refactor hierarchy-print, as I don't
>> understand what you think it ought to do differently once
>> hierarchy-print-line is added.
> 
> The goal is to have hierarchy-print keep its current behavior but
> leverage `hierarchy-print-line' for its implementation. I have something
> like this in mind (not tested):
> 
> (defun hierarchy-print (hierarchy &optional to-string)
>    "Insert HIERARCHY in current buffer as plain text.
> 
> Use TO-STRING to convert each element to a string.  TO-STRING is
> a function taking an item of HIERARCHY as input and returning a
> string.  If nil, TO-STRING defaults to a call to `format' with \"%s\"."
>    (hierarchy-print-line
>      hierarchy
>      (hierarchy-labelfn-indent
>        (lambda (item _) (insert (funcall to-string item) "\n")))))
> 
>> I also wonder if it's perhaps a little confusing to have an arg
>> 'to-string' that is now likely to be a call to
>> `hierarchy-labelfn-indent`, whereas in `hierarchy-print` `to-string`
>> is a function handed to hierarchy-labelfn-indent as an argument.
> 
> 
> hierarchy has a notion of "labeling function" (parameters of this type
> are called LABELFN). A labeling function is one that takes an item
> object and an indentation level number as arguments and returns a
> string. The `hierarchy-labelfn-indent' function takes a "labeling
> function" as argument and returns another one.
> 
> The function that `hierarchy-print' takes as parameter is called
> TO-STRING instead of LABELFN because it only accepts an item object as
> argument.
> 
> 
>> In that case, perhaps -print-line should have the same call to
>> `hierarchy-labelfn-indent` in its body, so that the to-string arg is
>> of a similar form to -print?
> 
> 
> I disagree. In `hierarchy-print-line', the second parameter should be
> named LABELFN instead of TO-STRING (sorry for not saying that
> earlier). The docstring should say that the function is not responsible
> for indentation and that the user is free to call
> `hierarchy-labelfn-indent' or `hierarchy-print' if s/he cares about
> indentation.
> 
> 
>> Finally, is there anything wrong with simply making the indent arg in
>> the lambda in my original suggestion '&optional'?
>>
>> i.e.
>>
>> (defun hierarchy-print (hierarchy &optional to-string indent-string)
>>     (let ((to-string (or to-string (lambda (item) (format "%s" item)))))
>>       (hierarchy-map
>>        (hierarchy-labelfn-indent (lambda (item &optional indent)
>>                                    (insert (funcall to-string item indent) "\n"))
>>                                  indent-string)
>>        hierarchy)))
> 
> The problem is not this lambda as `hierarchy-labelfn-indent' will always
> call it with 2 arguments anyway. The problem is with the TO-STRING
> parameter which used to be called with 1 argument but now requires
> 2. For example, the code below works before the change you suggest but
> triggers an error after:
> 
>    (hierarchy-print hierarchy (lambda (item) (format "-%s-")))
> 
> This is because the lambda here will now be called with 2 arguments but
> it only declares 1 parameter.
> 
> To go in the direction you suggest, we could use `func-arity' on
> TO-STRING to detect how many arguments we have to pass and then call it
> with either both ITEM and INDENT or only with ITEM.
> 
>> Maybe sending 'nil' as an argument to funcall is bad news?
> 
> 
> it's not the problem
> 
> 
>> I don't really understand why the -print function shouldn't take an
>> indent-string argument, but it's your pkg not mine.
> 
> 
> this is not the problem either. The problem is passing this argument to
> TO-STRING. I hope the above would have make this clearer.
> 

-- 
some writing: https://anarchive.mooo.com
an internets: https://pleasantlybabykid.tumblr.com/
.
xmpp: mousebot@ghost.noho.st
.
gpg pub key: 0x582C8EAF0B0D77C9
fingerprint: DA24 B943 36EF C491 E22F A70B 582C 8EAF 0B0D 77C9

[-- Attachment #2: hierarchy001.patch --]
[-- Type: text/x-patch, Size: 1488 bytes --]

diff --git a/hierarchy.el b/hierarchy.el
index 28fab55..fbfff04 100644
--- a/hierarchy.el
+++ b/hierarchy.el
@@ -475,10 +475,31 @@ indentation value (a number) as input."
 
 Use TO-STRING to convert each element to a string.  TO-STRING is
 a function taking an item of HIERARCHY as input and returning a
-string.  If nil, TO-STRING defaults to a call to `format' with \"%s\"."
+string.
+
+Calls `hierarchy-print-line' with `hierarchy-labelfn-indent' as
+second argument."
   (let ((to-string (or to-string (lambda (item) (format "%s" item)))))
+    (hierarchy-print-line
+     hierarchy
+     (hierarchy-labelfn-indent
+      (lambda (item _)
+        (funcall to-string item))))))
+
+(defun hierarchy-print-line (hierarchy &optional labelfn)
+  "Insert HIERARCHY in current buffer as plain text.
+
+Use LABELFN to convert each element to a string.  LABELFN is
+a function taking an item of HIERARCHY as input and returning a
+string.  If nil, LABELFN defaults to a call to `format' with \"%s\".
+
+This function is not responsible for indentation, but it can be
+achieved by providing a function such as
+`hierarchy-labelfun-indent' for LABELFN."
+  (let ((labelfn (or labelfn (lambda (item) (format "%s" item)))))
     (hierarchy-map
-     (hierarchy-labelfn-indent (lambda (item _) (insert (funcall to-string item) "\n")))
+     (lambda (item indent)
+       (insert (funcall labelfn item indent) "\n"))
      hierarchy)))
 
 (defun hierarchy-to-string (hierarchy &optional to-string)

Re: hierarchy-print doesn't allow sending indent-string arg to hierarchy-map

[Damien Cassou]

mousebot <mousebot@riseup.net> writes:
> perhaps there is some other code it run it against also?


I don't understand this sentence.


> a patch is attached, formatted with magit from a magit diff. is that
> applicable, or is a different format required?


Patches should usually be based on the root of the project so I would
expect to see "a/lisp/emacs-lisp/hierarchy.el" instead of
"a/hierarchy.el" in the patch. Also, the patch shouldn't be a plain diff
but should also contain information that the commit needs (e.g., author,
date, message).

To create a patch, my approach is to first create a dedicated branch,
then a commit (with a proper message) and finally to ask Magit to
generate a patch for the commit (with "M-x magit-patch-create" or "W c
c" and the cursor on the commit in the Magit status buffer).

Please read "** Commit messages" in the CONTRIBUTE file at the root of
the emacs tree to know how to write your commit message.

> --- a/hierarchy.el
> +++ b/hierarchy.el
> @@ -475,10 +475,31 @@ indentation value (a number) as input."
>  
>  Use TO-STRING to convert each element to a string.  TO-STRING is
>  a function taking an item of HIERARCHY as input and returning a
> -string.  If nil, TO-STRING defaults to a call to `format' with \"%s\"."
> +string.


I think the last sentence should be kept, it still makes sense.


> +
> +Calls `hierarchy-print-line' with `hierarchy-labelfn-indent' as
> +second argument."


This is an implementation detail we don't to mention here.

What is not an implementation detail is that the hierarchy's elements
are indented with a 2-space string. We could add a sentence about that.

We could also reference the new function `hierarchy-print-line' to tell
the user that this later one gives more configuration freedom but is a
little more complicated to use.


>    (let ((to-string (or to-string (lambda (item) (format "%s"
>    item)))))
> +    (hierarchy-print-line
> +     hierarchy
> +     (hierarchy-labelfn-indent
> +      (lambda (item _)
> +        (funcall to-string item))))))
> +
> +(defun hierarchy-print-line (hierarchy &optional labelfn)
> +  "Insert HIERARCHY in current buffer as plain text.
> +
> +Use LABELFN to convert each element to a string.  LABELFN is
> +a function taking an item of HIERARCHY as input


"as well as an indentation level as number".


> and returning a
> +string.  If nil, LABELFN defaults to a call to `format' with \"%s\".


I don't think LABELFN should have a default value in this function. It
is lower-level than hierarchy-print. Also, the user will have to
customize it in many cases to get indentation.


> +
> +This function is not responsible for indentation, but it can be
> +achieved by providing a function such as
> +`hierarchy-labelfun-indent' for LABELFN."
> +  (let ((labelfn (or labelfn (lambda (item) (format "%s" item)))))
>      (hierarchy-map
> -     (hierarchy-labelfn-indent (lambda (item _) (insert (funcall
> to-string item) "\n")))


indentation shouldn't be the responsibility of this function. This
function is lower-level.

Could you please also add a unit test or two for the new function?

Thank you for your work

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

hierarchy-print doesn't allow sending indent-string arg to hierarchy-map

[mousebot]

hi emacs,

`hierarchy-labelfn-indent' takes an optional second argument `indent-string'.

`hierarchy-print' calls `hierarchy-labelfn-indent', but doesn't allow providing it with an `indent-string' arg.

it also provides no second argument to the labelfn arg of `hierarchy-labelfn-indent'.
  
hierarchy-print looks like this:

```
(defun hierarchy-print (hierarchy &optional to-string)
   "Insert HIERARCHY in current buffer as plain text.

Use TO-STRING to convert each element to a string.  TO-STRING is
a function taking an item of HIERARCHY as input and returning a
string.  If nil, TO-STRING defaults to a call to `format' with \"%s\"."
   (let ((to-string (or to-string (lambda (item) (format "%s" item)))))
     (hierarchy-map
      (hierarchy-labelfn-indent (lambda (item _)
                                  (insert (funcall to-string item) "\n")))
      hierarchy)))
```

i'm working on a client and i'm using a copy of `hierarchy-print' that looks like this:

```
(defun hierarchy-print (hierarchy &optional to-string indent-string)
   "Insert HIERARCHY in current buffer as plain text.

Use TO-STRING to convert each element to a string.  TO-STRING is
a function taking an item of HIERARCHY as input and returning a
string.  If nil, TO-STRING defaults to a call to `format' with \"%s\"."
   (let ((to-string (or to-string (lambda (item) (format "%s" item)))))
     (hierarchy-map
      (hierarchy-labelfn-indent (lambda (item indent) ; add indent
                                  (insert (funcall to-string item indent) "\n")) ; add indent
                                indent-string) ; add indent-string
      hierarchy)))
```

i provide optional arg `indent-string' and hand it to `hierarchy-map'. i also provide a second 'indent' arg to the lambda that is the first arg for `hierarchy-labelfn-indent', indent is then given as the last arg of `funcall'. this allows me to print indented tree structures, which i was unable to achieve otherwise.

is there a reason this isn't the way `hierarchy-print' is coded? is there perhaps another way to achieve the same result? i have little experience with `hierarchy.el'.

regards,
m

Re: hierarchy-print doesn't allow sending indent-string arg to hierarchy-map

[Michael Heerdegen]

Hello,

Maybe Damien (the author) doesn't read this, I added an CC field.

mousebot <mousebot@riseup.net> writes:

> hi emacs,
>
> `hierarchy-labelfn-indent' takes an optional second argument
> `indent-string'.
>
> `hierarchy-print' calls `hierarchy-labelfn-indent', but doesn't allow
> providing it with an `indent-string' arg.
>
> it also provides no second argument to the labelfn arg of
> `hierarchy-labelfn-indent'.
>   
> hierarchy-print looks like this:
>
> ```
> (defun hierarchy-print (hierarchy &optional to-string)
>    "Insert HIERARCHY in current buffer as plain text.
>
> Use TO-STRING to convert each element to a string.  TO-STRING is
> a function taking an item of HIERARCHY as input and returning a
> string.  If nil, TO-STRING defaults to a call to `format' with \"%s\"."
>    (let ((to-string (or to-string (lambda (item) (format "%s" item)))))
>      (hierarchy-map
>       (hierarchy-labelfn-indent (lambda (item _)
>                                   (insert (funcall to-string item) "\n")))
>       hierarchy)))
> ```
>
>
> i'm working on a client and i'm using a copy of `hierarchy-print' that
> looks like this:
>
> ```
> (defun hierarchy-print (hierarchy &optional to-string indent-string)
>    "Insert HIERARCHY in current buffer as plain text.
>
> Use TO-STRING to convert each element to a string.  TO-STRING is
> a function taking an item of HIERARCHY as input and returning a
> string.  If nil, TO-STRING defaults to a call to `format' with \"%s\"."
>    (let ((to-string (or to-string (lambda (item) (format "%s" item)))))
>      (hierarchy-map
>       (hierarchy-labelfn-indent (lambda (item indent) ; add indent
>                                   (insert (funcall to-string item indent) "\n")) ; add indent
>                                 indent-string) ; add indent-string
>       hierarchy)))
> ```
>
> i provide optional arg `indent-string' and hand it to
> `hierarchy-map'. i also provide a second 'indent' arg to the lambda
> that is the first arg for `hierarchy-labelfn-indent', indent is then
> given as the last arg of `funcall'. this allows me to print indented
> tree structures, which i was unable to achieve otherwise.
>
> is there a reason this isn't the way `hierarchy-print' is coded? is
> there perhaps another way to achieve the same result? i have little
> experience with `hierarchy.el'.
>
> regards,
> m