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

[Ihor Radchenko <yantar92@gmail.com>]

Max Nikulin <manikulin@gmail.com> writes:

> I have rewritten the patch to use `pcase' and to fix allowed separators 
> between file name and node.

Thanks!

> +(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.

> +  (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'?

> +    (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 "  ".

> +      (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.

-- 
Ihor Radchenko,
Org mode contributor,
Learn more about Org mode at https://orgmode.org/.
Support Org development at https://liberapay.com/org-mode,
or support my work at https://liberapay.com/yantar92