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

2022-06-21 Thread Max Nikulin

On 21/06/2022 19:03, Hugo Heagren wrote:

+(defmacro test-ol-with-link-parameters-as (type parameters  body)

[...]

+  `(progn
+ (setq orig-parameters org-link-parameters)


I can easily miss something, but wouldn't it be enough to use let-binding

`(let ((org-link-parameters org-link-parameters))
   ,@body)

Otherwise it is better to use something like `condition-case' to restore 
original state even when some error is signaled.



+ (org-link-set-parameters ,type ,@parameters)
+ (let ((rtn (progn ,@body)))
+   (setq org-link-parameters orig-parameters)
+   rtn)))






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

2022-06-21 Thread Robert Pluim
> On Tue, 21 Jun 2022 13:03:32 +0100, Hugo Heagren  said:

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

They way your macro is defined, it uses `orig-parameters' in a way
that it will leak into the global namespace. Better would be to do

(let ((orig-parameters org-link-parameters))
   ...
  (setq org-link-parameters orig-parameters)

Robert
-- 



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

2022-06-21 Thread Hugo Heagren
I've finally had time to the `:default-description' parameter in
`org-link-parameters'.

Ihor Radchenko  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 " %S: "', since %S will
print a symbol, a string or a lambda correctly.

Hope this is a bit better!

Best,
Hugo
>From 5a44edfda4e09a95bba1327c2758b2637e5b16bd Mon Sep 17 00:00:00 2001
From: Hugo Heagren 
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 http://foo.com/(something)?after=parens link"
 
+;;; Insert Links
+
+(defmacro test-ol-with-link-parameters-as (type parameters  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 ( 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
--