emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Max Nikulin <manikulin@gmail.com>
To: emacs-orgmode@gnu.org
Subject: Re: [PATCH v2] ol-info: Define :insert-description function
Date: Fri, 19 Aug 2022 19:26:49 +0700	[thread overview]
Message-ID: <tdnvib$fjr$1@ciao.gmane.io> (raw)
In-Reply-To: <87zgg0q2kz.fsf@localhost>

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.



  reply	other threads:[~2022-08-19 12:28 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-28 23:15 ol.el: add description format parameter to org-link-parameters Hugo Heagren
2022-03-28 23:15 ` [PATCH] " Hugo Heagren
2022-04-04  9:49   ` Ihor Radchenko
2022-04-05 19:29     ` [PATCH v2] " Hugo Heagren
2022-04-07  5:13       ` Ihor Radchenko
2022-06-21 12:03         ` [PATCH v3] " Hugo Heagren
2022-06-21 13:41           ` Robert Pluim
2022-07-07 19:57             ` [PATCH v4] " Hugo Heagren
2022-07-09  3:31               ` Ihor Radchenko
2022-07-14 13:08                 ` [PATCH v5] " Hugo Heagren
2022-07-16  9:09                   ` Ihor Radchenko
2022-07-16 21:20                     ` Hugo Heagren
2022-07-17  6:11                       ` Max Nikulin
2022-07-17 10:27                         ` Ihor Radchenko
2022-07-17 10:18                       ` Ihor Radchenko
2022-07-17 20:59                         ` [PATCH v6] " Hugo Heagren
2022-07-18 10:55                           ` Max Nikulin
2022-07-23  7:48                             ` [PATCH v7] " Hugo Heagren
2022-07-23  7:59                               ` Max Nikulin
2022-07-23 13:06                                 ` Ihor Radchenko
2022-07-23 15:46                                   ` Max Nikulin
2022-07-24 10:34                                   ` Max Nikulin
2022-07-24 13:15                                     ` Ihor Radchenko
2022-07-25 11:55                                       ` [PATCH v8] " Hugo Heagren
2022-07-29 12:54                                         ` Max Nikulin
2022-07-29 19:05                                           ` [PATCH v9] " Hugo Heagren
2022-07-30  6:29                                             ` Ihor Radchenko
     [not found]                                               ` <87tu6zf2o1.fsf@heagren.com>
2022-07-30  8:02                                                 ` Ihor Radchenko
2022-07-30 12:34                                                   ` [PATCH] ol-info: Enable :insert-description feature Max Nikulin
2022-08-06  7:00                                                     ` Ihor Radchenko
2022-08-14 16:41                                                       ` [PATCH v2] ol-info: Define :insert-description function Max Nikulin
2022-08-19  4:28                                                         ` Ihor Radchenko
2022-08-19 12:26                                                           ` Max Nikulin [this message]
2022-08-20  7:29                                                             ` Ihor Radchenko
2022-08-21 14:49                                                               ` Max Nikulin
2022-08-22  4:10                                                                 ` Ihor Radchenko
2022-08-24 14:37                                                                   ` [PATCH v3] " Max Nikulin
2022-08-26 13:15                                                                     ` Ihor Radchenko
2022-09-04 15:05                                                       ` [PATCH] ol-info: Enable :insert-description feature Max Nikulin
2022-09-05  6:36                                                         ` Ihor Radchenko
2022-08-06  6:06                                             ` [PATCH v9] ol.el: add description format parameter to org-link-parameters Ihor Radchenko
2022-07-29  1:47                               ` [PATCH v7] " Ihor Radchenko
2022-07-29  7:05                                 ` Bastien Guerry
2022-07-10 10:26               ` [PATCH v4] " Max Nikulin
2022-06-21 15:01           ` [PATCH v3] " Max Nikulin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='tdnvib$fjr$1@ciao.gmane.io' \
    --to=manikulin@gmail.com \
    --cc=emacs-orgmode@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).