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

[Ihor Radchenko <yantar92@gmail.com>]

Hugo Heagren <hugo@heagren.com> writes:

> I've added the condition-case back to the check on
> `org-link-make-description', and added a new one to the check for the
> `:default-description' parameter, as Ihor suggested. I've also
> modified the handling of that parameter, to reflect
> `org-link-make-description', and updated the docstring accordingly.

Thanks!

Some more comments:

> +               (condition-case nil
> +                   (let ((def (org-link-get-parameter
> +                               type
> +                               :default-description)))
> +                     (cond
> +                      ((stringp def) def)
> +                      ((functionp def)
> +                       (funcall def link desc))))
> +                 (error
> +                  (message "Can't get link description from %S"
> +			   (symbol-name def))

Firstly, def will be undefined inside the error clause.

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.

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

> Apologies if the subject formatting is not correct, I'm still getting
> the hang of git-send-email.

You don't have to use git-send-email to submit simple patches. An easier
(as for me) alternative is a simple email with attached patch. magit
makes it trivial to create patch files.

Best,
Ihor