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

[Ihor Radchenko <yantar92@gmail.com>]

Hugo Heagren <hugo@heagren.com> writes:

> Since the last version of this patch, I have:
> - moved the code which sets `type' in `org-insert-link' to a position
>   where it covers more cases
> - rewritten the macros used in the tests, so that always (and
>   correctly) restore the original state after running, even after
>   errors. Thanks to Max Nikulin for suggesting using `condition-case'

Thanks!

> tl;dr The question is: what is the Good Behaviour when
> :default-description is set to something, which is meant to return a
> string and returns 'nil instead? Should it be treated like an empty
> string, or as an error?

Currently, the internal implementation will treat nil return value as if
there was no :default-description and org-link-make-description-function
were set to nil. We may probably document this. It sounds like a useful
behavior.

If the :default-description function returns non-string and not nil, the
behavior is simply undefined. It was also the case for
org-link-make-description-function. Though we might add a cl-assert
somewhere near the end of org-insert-link to deliberately throw an
error.

> 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'.

Could you also add the proper changelog entry for the test-ol.el file?

> +;;; 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'."
> +  ;; Copy all keys in `parameters' and their original values to
> +  ;; `orig-parameters'. For `parity': nil = odd, non-nill = even

*non-nil

Also, please end each sentence in the comment with ".".

> +  `(let (parity orig-parameters)
> +     (dolist (p ',parameters)
> +       (unless parity
> +         (setq orig-parameters
> +               (plist-put orig-parameters p (org-link-get-parameter ,type p))))
> +       (setq parity (not parity)))

This is a bit awkward code. You can instead use cl-loop by 'cddr or
a simple while loop with two (pop parameters) statements inside.

Also, ',parameters will fail if PARAMETERS argument is a variable name.

> +     ;; Set `parameters' values
> +     (condition-case err
> +         (let ((_ (org-link-set-parameters ,type ,@parameters))
> +               ;; Do body
> +               (rtn (progn ,@body)))
> +           ;; Restore original values
> +           (apply 'org-link-set-parameters ,type orig-parameters)
> +           ;; Return whatever the body returned
> +           rtn)
> +       ;; In case of error, restore state anyway AND really error
> +       (error
> +        (apply 'org-link-set-parameters ,type orig-parameters)
> +        (signal (car err) (cdr err))))))

unwind-protect is more suitable here and will lead to more concise code.

> 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.

Please start sentences after ":" with capital letter. Also, you missed
double space between sentences.

> +`: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'

I think that we should also document the new parameter in ORG-NEWS.
  
Best,
Ihor