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

[Hugo Heagren]

> Can you also update the documentation for
> org-link-make-description-function?

I'm not sure what sort of documentation you have in mind? What should
I add?

> I have realized that even better option would be using map-do.

Agreed. Should be in the current version.

With regard to Max's comments:

> I am in doubts concerning "default-description" as the parameter
> name. I would consider either more specific "insert-description"

Having read your thoughts, I agree. I've converted the patch to use
"insert-description" where-ever relevant.

> Have I missed something or the whole macro may be simplified to just
> copy `org-link-parameters'?
>
>    `(let ((org-link-parameters (copy-tree org-link-parameters)))
>       (org-link-set-parameters ,type ,@parameters)
>       ,@body))

I had the same thought. I doesn't work though. `copy-tree' and
`copy-sequence' only make shallow copies of each element in a
sequence. `org-link-set-parameters' then uses side effects to alter
the `org-link-parameters', so these changes are propagated to the
copy. This means the values in the copy and the real
`org-link-parameters' are always the same, and so we can't use the
copy to store the original values (which is what we need it to do).

> In addition, `declare' form should be added to `defmacro' to specify
> which argument is considered as its body.

I have never used `declare' before. I looked it up in the Elisp manual
and read the docstring, but I couldn't understand how it specifies
which argument is considered the body. I'm also not aware of what this
does/why this is useful? (not a criticism, I just haven't learned this
yet).

> My opinion is that it should be inside
>    (let ((initial-input ...

This is a good idea -- done.

> and maybe even be a sibling of
>     (def (org-link-get-parameter type
> to keep related code more local.

This isn't possible, because that's a clause in a `let' call, which is
itself inside a `cond' clause, and the `CONDITION' of that clause uses
`type', so it has to be defined at a higher level.

> I have not tested, so I may be wrong in respect to the following. It
> seems, you rises priority of desc value that earlier was a fallback.
> The reason why I am in doubts, is that it seems, desc is initialized
> from current link description when point is withing an existing link
> and in such cases desc likely should be preserved. I can not say
> that I like that it is responsibility of all description functions
> to return the desc argument if it is supplied.

It's right that `desc' is initialized from current link description
when point is withing an existing link.

Previously, `desc' was only used when
`org-link-make-description-function' was nil. This seems to me a very
odd behaviour: preserve the current link description, but only when
`org-link-make-description-function' is nil. Especially considering
that `org-insert-link' is also used for editing already-existing
links. So in the original version, in some situations,
`org-link-make-description-function' had a higher priority than
`desc', which seems wrong.

Thanks,

Hugo

0001-ol.el-add-description-format-parameter-to-org-link-p.patch
Description: Text Data

0002-test-ol-tests-for-insert-description-param-when-inse.patch
Description: Text Data