Re: [PATCH] org-id: implement arbitrary cross-file references

[Ruijie Yu]

Thanks for the patch.  Some in-line comments below.

Sergei Kosyrev <kosejin@proton.me> writes:

> From f712fa57a90c68d3d9066b10f49822ea0337b923 Mon Sep 17 00:00:00 2001
> From: Kosyrev Serge <serge.kosyrev@iohk.io>
> Date: Thu, 25 May 2023 19:30:01 +0800
> Subject: [PATCH] org-id: implement arbitrary cross-file references
> 
> * Table formulae can now refer to data from tables in other files.
> 
> TINYCHANGE

First of all, I believe this is not within the scope of TINYCHANGE,
because as you see below, you added 58 lines and removed 11.  In this
case, the very first thing required before Org maintainers can consider
its inclusion is that you sign the FSF copyright assignment.  If you
need more details, Ihor or Bastien could probably elaborate and/or send
you the form.

> ---
>  etc/ORG-NEWS      | 10 +++++++++
>  lisp/org-id.el    | 57 ++++++++++++++++++++++++++++++++++++++---------
>  lisp/org-table.el |  2 +-
>  3 files changed, 58 insertions(+), 11 deletions(-)
> 
> diff --git a/lisp/org-id.el b/lisp/org-id.el
> index aa9610f16..2fcecbb50 100644
> --- a/lisp/org-id.el
> +++ b/lisp/org-id.el
> @@ -337,6 +337,40 @@ Move the cursor to that entry in that buffer."
>      (move-marker m nil)
>      (org-fold-show-context)))
>  
> +(defun org-id-parse-remote-table-ref (refstr)
> +  (let ((match (string-match "^file:\\([^:]+\\)\\(\\|:.+\\)$" refstr)))
> +    (unless (null match)
> +      (let* ((m1 (match-string 1 refstr))
> +             (m2 (match-string 2 refstr))
> +             (filename (cl-remove-if (lambda (c) (member c '(40 41)))
> +                                  (org-table-formula-substitute-names m1)))
> +             (table-name (org-table-formula-substitute-names m2)))
> +        (list filename table-name)))))

First, do we need to wrap it with `save-match-data'?  In my personal
code I almost always do, but I need a second opinion on that.

Second, instead of the let-unless-null combination, you can just do this
instead:

    (when-let ((match ...))
      (let* (...)
        ...))

And, if those values can't ever be nil, you could even combine the
entirety of the `let*' form into the `when-let' form (and turn it into a
`when-let*' form).

No comments on the rest, so I didn't quote them from my response to save
bandwidth.

-- 
Best,


RY