[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] ol-info: Define :insert-description function
From: |
Max Nikulin |
Subject: |
Re: [PATCH v2] ol-info: Define :insert-description function |
Date: |
Fri, 19 Aug 2022 19:26:49 +0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 |
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.
- Re: [PATCH] ol-info: Enable :insert-description feature, Ihor Radchenko, 2022/08/06
- [PATCH v2] ol-info: Define :insert-description function, Max Nikulin, 2022/08/14
- Re: [PATCH v2] ol-info: Define :insert-description function, Ihor Radchenko, 2022/08/19
- Re: [PATCH v2] ol-info: Define :insert-description function,
Max Nikulin <=
- Re: [PATCH v2] ol-info: Define :insert-description function, Ihor Radchenko, 2022/08/20
- Re: [PATCH v2] ol-info: Define :insert-description function, Max Nikulin, 2022/08/21
- Re: [PATCH v2] ol-info: Define :insert-description function, Ihor Radchenko, 2022/08/22
- Re: [PATCH v3] ol-info: Define :insert-description function, Max Nikulin, 2022/08/24
- Re: [PATCH v3] ol-info: Define :insert-description function, Ihor Radchenko, 2022/08/26