Re: [PATCH v3] ol.el: add description format parameter to org-link-parameters

[Hugo Heagren <hugo@heagren.com>]

[-- Attachment #1: Type: text/plain, Size: 1594 bytes --]

I've finally had time to the `:default-description' parameter in
`org-link-parameters'.

Ihor Radchenko <yantar92@gmail.com> writes:

> I recommend writing tests for org-insert-link in
> testing/lisp/test-ol.el and testing expected behaviour for various
> values of :default-description.

Good advice! I haven't written many tests before, so this was a
challenge. I've attached a patch with some tests for the expected
behaviour. These include a couple of macros and a function, to stop
the definitions being too huge and unwieldy.

All the tests pass on my machine.

They're very nearly just right, but for some reason
`test-ol-with-link-parameters-as' doesn't always reset the parameters
correctly for me. However I have them set to originally, it sets
`:default-description' to a lambda. This might be a quirk at my end
though -- if someone else could have a look I'd be very grateful.

The tests showed that my previous approach doesn't work generally
enough. I've moved where `type' is defined in the code, to cover all
cases.

> Firstly, def will be undefined inside the error clause.

I've defined def a bit further up in a `let', and run the error clause
inside this.

> Secondly, when :default-description is a lambda expression and that
> expression fails, your code will fail with "wrong-type-argument
> symbolp" when executing (symbol-name def). Please consider cases
> when `def' is not a string and not a function symbol.

Dealt with this by just using `message "<text> %S: "', since %S will
print a symbol, a string or a lambda correctly.

Hope this is a bit better!

Best,
Hugo

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-test-ol-tests-for-default-description-param-when-ins.patch --]
[-- Type: text/x-diff, Size: 3476 bytes --]

From 5a44edfda4e09a95bba1327c2758b2637e5b16bd Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Tue, 21 Jun 2022 12:45:50 +0100
Subject: [PATCH 2/2] test-ol: tests for default-description param when
 inserting links

Add tests for various values of `:default-description' in
`org-link-parameters'.
---
 testing/lisp/test-ol.el | 77 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/testing/lisp/test-ol.el b/testing/lisp/test-ol.el
index 429bb52ee..7524d73af 100644
--- a/testing/lisp/test-ol.el
+++ b/testing/lisp/test-ol.el
@@ -625,5 +625,82 @@ See https://github.com/yantar92/org/issues/4."
     (test-ol-parse-link-in-text
         "The <point>http://foo.com/(something)?after=parens link"))))
 
+;;; Insert Links
+
+(defmacro test-ol-with-link-parameters-as (type parameters &rest body)
+  "Pass TYPE/PARAMETERS to `org-link-parameters' and execute BODY.
+
+Save the original value of `org-link-parameters', execute
+`org-link-set-parameters' with the relevant args, execute BODY
+and restore `org-link-parameters'.
+
+TYPE is as in `org-link-set-parameters'.  PARAMETERS is a plist to
+be passed to `org-link-set-parameters'."
+  `(progn
+     (setq orig-parameters org-link-parameters)
+     (org-link-set-parameters ,type ,@parameters)
+     (let ((rtn (progn ,@body)))
+       (setq org-link-parameters orig-parameters)
+       rtn)))
+
+(defun test-ol-insert-link-get-desc (&optional link-location description)
+  "Insert link in temp buffer, return description.
+
+LINK-LOCATION and DESCRIPTION are passed to
+`org-insert-link' (COMPLETE-FILE is always nil)."
+  (org-test-with-temp-text ""
+    (org-insert-link nil link-location description)
+    (save-match-data
+      (when (and
+             (org-in-regexp org-link-bracket-re 1)
+             (match-end 2))
+        (match-string-no-properties 2)))))
+
+(defun test-ol/return-foobar (_link-test _desc)
+  "Return string \"foobar\".
+
+Take (and ignore) arguments conforming to `:default-description'
+API in `org-link-parameters'.  Used in test
+`test-ol/insert-link-default-description', for the case where
+`:default-description' is a function symbol."
+  "foobar")
+
+(ert-deftest test-ol/insert-link-default-description ()
+  "Test `:default-description' parameter handling."
+  ;; String case
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description "foobar")
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Lambda case
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description (lambda (_link-test _desc) "foobar"))
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Function symbol case
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description #'test-ol/return-foobar)
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; `:default-description' parameter is defined, but doesn't return a
+  ;; string
+  (should-error
+   (test-ol-with-link-parameters-as
+    "id" (:default-description #'ignore)
+    (test-ol-insert-link-get-desc "id:foo-bar")))
+  ;; Description argument should override `:default-description'
+  (should
+   (string=
+    "notfoobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description "foobar")
+     (test-ol-insert-link-get-desc "id:foo-bar" "notfoobar")))))
+
 (provide 'test-ol)
 ;;; test-ol.el ends here
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-ol.el-add-description-format-parameter-to-org-link-p.patch --]
[-- Type: text/x-diff, Size: 4870 bytes --]

From 437155c2394b5c9961ecc0af4e1e1a54b63416fe Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Mon, 28 Mar 2022 23:18:45 +0100
Subject: [PATCH 1/2] ol.el: add description format parameter to
 org-link-parameters

* ol.el (org-link-parameters): add parameter `:default-description', a
string or a function.
* (org-insert-link): if no description is provided (pre-existing or as
an argument), next option is to use the `:default-description' (if
non-nil) parameter to generate one.

Default descriptions are predictable within a link type, but because
link types are quite diverse, are NOT predictable across many types. A
type-parameter is thus a good place to store information on the
default description.
---
 lisp/ol.el | 47 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/lisp/ol.el b/lisp/ol.el
index d4bd0e40c..1ab3a5f76 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -141,6 +141,15 @@ link.
   Function that inserts a link with completion.  The function
   takes one optional prefix argument.
 
+`:default-description'
+
+  String or function used as a default when prompting users for a
+  link's description.  A string is used as-is, a function is
+  called with two arguments: the full link text, and the
+  description generated by `org-insert-link'.  It should return
+  the description to use (this reflects the behaviour of
+  `org-link-make-description-function').
+
 `:display'
 
   Value for `invisible' text property on the hidden parts of the
@@ -1804,11 +1813,14 @@ prefix negates `org-link-keep-stored-after-insertion'.
 If the LINK-LOCATION parameter is non-nil, this value will be used as
 the link location instead of reading one interactively.
 
-If the DESCRIPTION parameter is non-nil, this value will be used as the
-default description.  Otherwise, if `org-link-make-description-function'
-is non-nil, this function will be called with the link target, and the
-result will be the default link description.  When called non-interactively,
-don't allow to edit the default description."
+If the DESCRIPTION parameter is non-nil, this value will be used
+as the default description.  If not, and the chosen link type has
+a non-nil `:default-description' parameter, that is used to
+generate a description as described in `org-link-parameters'
+docstring. Otherwise, if `org-link-make-description-function' is
+non-nil, this function will be called with the link target, and
+the result will be the default link description.  When called
+non-interactively, don't allow to edit the default description."
   (interactive "P")
   (let* ((wcf (current-window-configuration))
 	 (origbuf (current-buffer))
@@ -1818,7 +1830,7 @@ don't allow to edit the default description."
 	 (desc region)
 	 (link link-location)
 	 (abbrevs org-link-abbrev-alist-local)
-	 entry all-prefixes auto-desc)
+	 entry all-prefixes auto-desc type)
     (cond
      (link-location)		      ; specified by arg, just use it.
      ((org-in-regexp org-link-bracket-re 1)
@@ -1893,6 +1905,13 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
       (or entry (push link org-link--insert-history))
       (setq desc (or desc (nth 1 entry)))))
 
+    (setq type
+          (cond
+           ((string-match (rx (: string-start (submatch (eval `(or ,@all-prefixes))) ":")) link)
+            (match-string 1 link))
+           ((file-name-absolute-p link) "file")
+           ((string-match "\\`\\.\\.?/" link) "file")))
+
     (when (funcall (if (equal complete-file '(64)) 'not 'identity)
 		   (not org-link-keep-stored-after-insertion))
       (setq org-stored-links (delq (assoc link org-stored-links)
@@ -1961,12 +1980,24 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
       (let ((initial-input
 	     (cond
 	      (description)
-	      ((not org-link-make-description-function) desc)
+              (desc)
+              ((org-link-get-parameter type :default-description)
+               (let ((def (org-link-get-parameter type :default-description)))
+                 (condition-case nil
+                     (cond
+                      ((stringp def) def)
+                      ((functionp def)
+                       (funcall def link desc)))
+                   (error
+                    (message "Can't get link description from org link parameter `:default-description': %S"
+			     def)
+		    (sit-for 2)
+                    nil))))
 	      (t (condition-case nil
 		     (funcall org-link-make-description-function link desc)
 		   (error
 		    (message "Can't get link description from %S"
-			     (symbol-name org-link-make-description-function))
+		             org-link-make-description-function)
 		    (sit-for 2)
 		    nil))))))
 	(setq desc (if (called-interactively-p 'any)
-- 
2.20.1