Re: [PATCH v2] ol-info: Define :insert-description function

[Max Nikulin]

On 19/08/2022 11:28, Ihor Radchenko wrote:
> Max Nikulin writes:
>
> > +(defun org-info--link-file-node (path)
> > +  "Extract file name and node from info link PATH.
> > +
> > +Return cons consisting of file name and node name or \"Top\" if node
> > +part is not specified. Components may be separated by \":\" or by \"#\"."
>
> It looks like the docstring does not match what the function actually
> returns.

It returns a cons, doesn't it? Is it confusing that separator for 
components is related to the argument?

> > +  (if (not path)
> > +      '("dir" . "Top")
>
> "dir" is not a file. Also, it is not very clear what "dir" is referring
> to. Maybe you can add a comment pointing to `org-info-other-documents'?

Try M-x info RET when you do not have *info* buffer and you get "(dir) 
Top" node. It is the directory of info files. If you run "info" without 
argument in shell you will get the same.

> > +    (string-match "\\`\\([^#:]*\\)\\(?:[#:]:?\\(.*\\)\\)?\\'" path)
> > +    (let* ((node (match-string 2 path))
> > +           ;; `string-trim' modifies match
>
> Here and is several other places, including docstrings, please make sure
> that the sentences end with "." and are separated with "  ".

It was supposed to be a brief phrase rather than complete sentence.

> > +      (cons
> > +       ;; Fallback to "org" is an arbirtrary choice
> > +       ;; and added because "(dir)filename" does not work as "filename".
>
> Should this be documented? Or at least mentioned that the behaviour is
> undefined. And if it is undefined you should not test it in the tests.

The purpose of test is to check that it does not signal some obscure 
error. I am unsure how to handle corner cases, so I am open to 
suggestions. Some considerations
- `org-info--link-file-node' may return nil when link path is incomplete 
or some value that may help to avoid error handling code paths in callers.
- <info:> does not look like a valid link but it may be handled like 
shell "info" command without argument, so I chose "(dir)" node. Elisp 
(info) without arguments however may display existing buffer.
- <info:dir> certainly should be handled as (info "(dir)")
- <info:dir#elisp> is invalid. Maybe (info "elisp") should be used instead.
- <info:#Tables> I am unsure in my choice to open (info "(org) Tables"). 
Maybe it is better to treat it as "(dir) Tables" and so as "(dir) Top" 
node since "(dir) Top" is quite reasonable for <info:> with empty path.

Thanks for the review.