Re: [PATCH] org-id: allow using parent's existing id in links to headlines

[Ihor Radchenko <yantar92@posteo.net>]

"Rick Lupton" <mail@ricklupton.name> writes:

> Here's an updated patch, which adds (optional) search strings to ID links, and the option to inherit ID targets from parent headline / the top level file properties.  I've also updated ORG-NEWS and the manual, and added tests.
>
> I think I've fixed all the issues with my first patch about which headline gets used for the description when inheriting IDs, what happens if there is no ID, etc.

Thanks!

> +*** ~org-id-store-link~ now adds search strings for precise link targets
> +
> +Previously, search strings are supported for =file:= links but not for
> +=id:= links.  This change adds support for search strings to =id:=
> +links.

"This change..." part sounds like a commit message, not a NEWS item.
You may probably remove this paragraph.

>  \f
> +(defun org-link--try-link-store-functions (interactive?)
> +  "Try storing external links, prompting if more than one is possible.
> +
> +Each function returned by `org-store-link-functions` is called in
> +turn. If multiple functions return non-nil, prompt for which link
> +should be stored.

We use `...' Elisp quoting, not markdown.
Also, please keep two spaces between sentences - you do it
inconsistently across the patch.

> +Return t if a function has successfully stored a link, which will
> +be stored in `org-link-store-props`.

I'd better say "Return t when the link is stored in `org-store-link-plist'."

> +(defcustom org-id-link-consider-parent-id nil
> +  "Non-nil means storing a link to an Org file will consider parent entry IDs.
> +
> +Use this with `org-id-link-use-context` set to `t` to allow
> +linking to uniquely-named sub-entries within a parent entry with
> +an ID, without requiring every sub-entry to have its own ID."

1. `...' quoting
2. The docstring is slightly confusing. Having an example would be helpful.

> +(defcustom org-id-link-use-context t
> +  "Non-nil means org-id links from `org-id-store-link' contain context.
> +\\<org-mode-map>
> +A search string is added to the id with \"::\" as separator and
> +used to find the context when the link is activated by the
> +command `org-open-at-point'.  When this option is t, the entire
> +active region is be placed in the search string of the file link.
> +If set to a positive integer N, only the first N lines of context
> +are stored.

It does not look like integer value is respected in the patch.

> +Using a prefix argument to the commands `org-store-link' \
> +\(`\\[universal-argument] \\[org-store-link]') or
> +`org-id-store-link` negates this setting for the duration of the
> +command."

You should also update the docstring of `org-store-link' accordingly.

>  ;;;###autoload
> -(defun org-id-get-create (&optional force)
> +(defun org-id-get-create (&optional force inherit)
>...
>  ;;;###autoload
> -(defun org-id-get (&optional epom create prefix)
> +(defun org-id-get (&optional epom create prefix inherit)

Please document this new optional arguments in the NEWS.

> -  (let ((id (org-entry-get epom "ID")))
> +  (let ((id (org-entry-get epom "ID" inherit)))

This makes your description of INHERIT argument slightly inaccurate - for
`org-entry-get', INHERIT can also be a special symbol 'selective.

> -(defun org-id-store-link ()
> +(defun org-id-store-link (interactive?)

Please make this new argument optional and document the argument in the
docstring and NEWS. Non-optional new argument is a breaking change that
may break third-party code.

>    "Store a link to the current entry, using its ID.
>  
> +See also `org-id-link-to-org-use-id`,
> +`org-id-link-use-context`,
> +`org-id-link-consider-parent-id`.
> +
>  If before first heading store first title-keyword as description
>  or filename if no title."
> -  (interactive)
> -  (when (and (buffer-file-name (buffer-base-buffer)) (derived-mode-p 'org-mode))
> -    (let* ((link (concat "id:" (org-id-get-create)))
> +  (interactive "p")
> +  (when (and (buffer-file-name (buffer-base-buffer))
> +             (derived-mode-p 'org-mode)
> +             (or (eq org-id-link-to-org-use-id t)

I do not like this change - `org-id-store-link' is not only used by
`org-store-link'. Suddenly honoring `org-id-link-to-org-use-id' will be
a breaking change. Instead, I suggest you to write a wrapper function,
like `org-id-store-link-maybe' and use it as :store id link property.
That function will call `org-id-store-link' as necessary according to
user customization.

> +      ;; Prefix to `org-store-link` negates preference from `org-id-link-use-context`.
> +      (when (org-xor current-prefix-arg org-id-link-use-context)

This is not reliable. `org-id-store-link' may be called from a completely
different command, not `org-store-link'. Then, the effect of prefix
argument will be unexpected. You should instead process prefix argument
right in `org-store-link' by let-binding `org-id-link-use-context'
around the call to `org-id-store-link'.

> +        (pcase (org-link-precise-link-target id-location)

Why not passing the RELATIVE-TO argument?

>  (defun org-id-open (id _)
>    "Go to the entry with id ID."
> -  (org-mark-ring-push)
> -  (let ((m (org-id-find id 'marker))
> -	cmd)
> +  (let* ((option (and (string-match "::\\(.*\\)\\'" id)
> +		      (match-string 1 id)))
> +	 (id (if (not option) id
> +               (substring id 0 (match-beginning 0))))
> +         m cmd)
> +    (org-mark-ring-push)
> +    (setq m (org-id-find id 'marker))

This means that the existing IDs that happen to contain :: will be
broken. For such IDs, we should (1) document the problem in the news;
(2) try harder to match them calling `org-id-find' with all the possible
ID values until one matches.

> +    (when option
> +      (org-link-search option))
>      (org-fold-show-context)))

`org-link-search' does not always search from point. So, you may end up
matching, for example, a duplicate CUSTOM_ID above.
Moreover, regular expression match option will be broken -
`org-link-search' creates sparse tree in the whole buffer and will
disregard the ID part of the link. I suspect that you will need to make
dedicated modifications to `org-link-search' as well in order to
implement opening ID links with search option cleanly.

-- 
Ihor Radchenko // yantar92,
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>