Hugo Heagren <h...@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