Public API change: How to handle function signature change gracefully

Public API change: How to handle function signature change gracefully

[Benjamin Andresen via General discussions about Org-mode.]

Hello everyone,

I would like to change the public API of the :face part of `org-link-set-parameters':

        (org-link-set-parameters "file" :face 'org-link)

My ultimate goal is to have org-links be able to be have their face changed based on the contents, not just the path of the link.

I found the relevant code in org.el in the function `org-activate-links':

   'face (pcase (org-link-get-parameter type :face)
   ((and (pred functionp) face) (funcall face path))
   ((and (pred facep) face) face)
   ((and (pred consp) face) face) ;anonymous
   (_ 'org-link))

and would like to change this to 

   'face (pcase (org-link-get-parameter type :face)
   ((and (pred functionp) face) (funcall face path contents)) ;; this is the change
   ((and (pred facep) face) face)
   ((and (pred consp) face) face) ;anonymous
   (_ 'org-link))

Now that will introduce a host of call issues because the callees don't expect to the amount of arguments changed under their bottom.

I would like some guidance how I could get what I think is neat: The contents of the bracket-style links as an additional parameter to set faces on and not breaking existing hookups.

I'm thinking to change the above code to look at the callee's function signature and checking how many arguments it accepts and then use the right funcall for the situation.

To make this more palatable I would suggest it's changed so that the 2nd argument will be a list of alists or keywords so to not have this function signature problem if someone else comes up with a reason to introduce yet more data.

Thanks in advance,
Benjamin Andresen

Re: Public API change: How to handle function signature change gracefully

[Nicolas Goaziou]

Hello,

Benjamin Andresen writes:
>
> I would like to change the public API of the :face part of `org-link-set-parameters':
>
>         (org-link-set-parameters "file" :face 'org-link)
>
> My ultimate goal is to have org-links be able to be have their face changed based on the contents, not just the path of the link.
>
> I found the relevant code in org.el in the function `org-activate-links':
>
>    'face (pcase (org-link-get-parameter type :face)
>    ((and (pred functionp) face) (funcall face path))
>    ((and (pred facep) face) face)
>    ((and (pred consp) face) face) ;anonymous
>    (_ 'org-link))
>
> and would like to change this to 
>
>    'face (pcase (org-link-get-parameter type :face)
>    ((and (pred functionp) face) (funcall face path contents)) ;; this is the change
>    ((and (pred facep) face) face)
>    ((and (pred consp) face) face) ;anonymous
>    (_ 'org-link))
>
> Now that will introduce a host of call issues because the callees don't expect to the amount of arguments changed under their bottom.
>
> I would like some guidance how I could get what I think is neat: The
> contents of the bracket-style links as an additional parameter to set
> faces on and not breaking existing hookups.

Isn't the function called with point on the link? You may just need to
extract the contents from the environment.

Otherwise, a solution is to catch `wrong-number-of-arguments' error and
call again the function with the old calling convention. See, e.g.,
`org-link-open'.

> To make this more palatable I would suggest it's changed so that the
> 2nd argument will be a list of alists or keywords so to not have this
> function signature problem if someone else comes up with a reason to
> introduce yet more data.

Beware the over-engineering. At this point, someone motivated enough can
put an advice to the link fontification function.

Regards,

-- 
Nicolas Goaziou

Re: Public API change: How to handle function signature change gracefully

[John Kitchin]

[-- Attachment #1: Type: text/plain, Size: 3184 bytes --]

I think what Nicolas suggests is probably the easiest path. Here is one
example that does what I think you are looking for. I use a simple string
comparison on the contents, you could do something more sophisticated.

#+BEGIN_SRC emacs-lisp
(defun fruit-link-face (path)
  (let* ((ln (org-element-context))
(start (org-element-property :contents-begin ln))
(end (org-element-property :contents-end ln))
(contents (if (and start end)
      (buffer-substring start end)
    nil)))
    (if (and contents (stringp contents))
(if (string> contents "j")
   '(:foreground "red")
 '(:foreground "blue"))
      'org-link)))


(org-link-set-parameters "fruit"
                         :face 'fruit-link-face)
#+END_SRC

#+RESULTS:
| :face | fruit-link-face |

[[fruit:mango ][test]].  # this will be red

  fruit:apple. # regular org link.

 [[fruit:apple][bera]]. # this will be blue
John

-----------------------------------
Professor John Kitchin
Doherty Hall A207F
Department of Chemical Engineering
Carnegie Mellon University
Pittsburgh, PA 15213
412-268-7803
@johnkitchin
http://kitchingroup.cheme.cmu.edu



On Tue, Apr 21, 2020 at 1:57 PM Nicolas Goaziou <mail@nicolasgoaziou.fr>
wrote:

> Hello,
>
> Benjamin Andresen writes:
> >
> > I would like to change the public API of the :face part of
> `org-link-set-parameters':
> >
> >         (org-link-set-parameters "file" :face 'org-link)
> >
> > My ultimate goal is to have org-links be able to be have their face
> changed based on the contents, not just the path of the link.
> >
> > I found the relevant code in org.el in the function `org-activate-links':
> >
> >    'face (pcase (org-link-get-parameter type :face)
> >    ((and (pred functionp) face) (funcall face path))
> >    ((and (pred facep) face) face)
> >    ((and (pred consp) face) face) ;anonymous
> >    (_ 'org-link))
> >
> > and would like to change this to
> >
> >    'face (pcase (org-link-get-parameter type :face)
> >    ((and (pred functionp) face) (funcall face path contents)) ;; this is
> the change
> >    ((and (pred facep) face) face)
> >    ((and (pred consp) face) face) ;anonymous
> >    (_ 'org-link))
> >
> > Now that will introduce a host of call issues because the callees don't
> expect to the amount of arguments changed under their bottom.
> >
> > I would like some guidance how I could get what I think is neat: The
> > contents of the bracket-style links as an additional parameter to set
> > faces on and not breaking existing hookups.
>
> Isn't the function called with point on the link? You may just need to
> extract the contents from the environment.
>
> Otherwise, a solution is to catch `wrong-number-of-arguments' error and
> call again the function with the old calling convention. See, e.g.,
> `org-link-open'.
>
> > To make this more palatable I would suggest it's changed so that the
> > 2nd argument will be a list of alists or keywords so to not have this
> > function signature problem if someone else comes up with a reason to
> > introduce yet more data.
>
> Beware the over-engineering. At this point, someone motivated enough can
> put an advice to the link fontification function.
>
> Regards,
>
> --
> Nicolas Goaziou
>
>

[-- Attachment #2: Type: text/html, Size: 4286 bytes --]

Re: Public API change: How to handle function signature change gracefully

[Benjamin Andresen via General discussions about Org-mode.]

Thanks John and Nicolas.

I sort of arrived at the same point with my over-engineered approach and change of public API.

modified   lisp/org.el
@@ -5081,10 +5081,18 @@ This includes angle, plain, and bracket links."
(link (org-element-property :raw-link link-object))
(type (org-element-property :type link-object))
(path (org-element-property :path link-object))
+		 (cbeg (org-element-property :contents-begin link-object))
+		 (cend (org-element-property :contents-end link-object))
+		 (contents (if (and cbeg cend)
+			       (buffer-substring-no-properties cbeg cend)))
(properties		;for link's visible part
  (list
   'face (pcase (org-link-get-parameter type :face)
-			   ((and (pred functionp) face) (funcall face path))
+			   ((and (pred functionp) face) (let* ((number-of-args (cdr (func-arity face)))
+							       (kws (plist-put '() :contents contents)))
+							  (if (equal number-of-args 1)
+							      (funcall face path)
+							    (funcall face path kws))))
   ((and (pred facep) face) face)
   ((and (pred consp) face) face) ;anonymous
   (_ 'org-link))

This in turn resulted in more complexity on the callee side as I had to resolve the keywords.

So thanks for this simple fix I get rid of my over-engineered proposal: Having access to `org-element-context' is great for my usecase, because it means it will work on versions of Org that are already released.

Yay!

Best regards,
Benny


Apr 21, 2020, 22:07 by jkitchin@andrew.cmu.edu:

> I think what Nicolas suggests is probably the easiest path. Here is one example that does what I think you are looking for. I use a simple string comparison on the contents, you could do something more sophisticated.
>
> #+BEGIN_SRC emacs-lisp
> (defun fruit-link-face (path)
>   (let* ((ln (org-element-context))
>  (start (org-element-property :contents-begin ln))
>  (end (org-element-property :contents-end ln))
>  (contents (if (and start end)
>        (buffer-substring start end)
>      nil)))
>     (if (and contents (stringp contents))
>  (if (string> contents "j")
>     '(:foreground "red")
>   '(:foreground "blue"))
>       'org-link)))
>
>
> (org-link-set-parameters "fruit"
>                          :face 'fruit-link-face)
> #+END_SRC
>
> #+RESULTS:
> | :face | fruit-link-face |
>
> [[fruit:mango ][test]].  # this will be red
>
>   fruit:apple. # regular org link.
>
>  [[fruit:apple][bera]]. # this will be blue
> John
>
> -----------------------------------
> Professor John Kitchin 
> Doherty Hall A207F
> Department of Chemical Engineering
> Carnegie Mellon University
> Pittsburgh, PA 15213
> 412-268-7803
> @johnkitchin
> http://kitchingroup.cheme.cmu.edu
>
>
> On Tue, Apr 21, 2020 at 1:57 PM Nicolas Goaziou <> mail@nicolasgoaziou.fr> > wrote:
>
>> Hello,
>>  
>>  Benjamin Andresen writes:
>>  >
>>  > I would like to change the public API of the :face part of `org-link-set-parameters':
>>  >
>>  >         (org-link-set-parameters "file" :face 'org-link)
>>  >
>>  > My ultimate goal is to have org-links be able to be have their face changed based on the contents, not just the path of the link.
>>  >
>>  > I found the relevant code in org.el in the function `org-activate-links':
>>  >
>>  >    'face (pcase (org-link-get-parameter type :face)
>>  >    ((and (pred functionp) face) (funcall face path))
>>  >    ((and (pred facep) face) face)
>>  >    ((and (pred consp) face) face) ;anonymous
>>  >    (_ 'org-link))
>>  >
>>  > and would like to change this to 
>>  >
>>  >    'face (pcase (org-link-get-parameter type :face)
>>  >    ((and (pred functionp) face) (funcall face path contents)) ;; this is the change
>>  >    ((and (pred facep) face) face)
>>  >    ((and (pred consp) face) face) ;anonymous
>>  >    (_ 'org-link))
>>  >
>>  > Now that will introduce a host of call issues because the callees don't expect to the amount of arguments changed under their bottom.
>>  >
>>  > I would like some guidance how I could get what I think is neat: The
>>  > contents of the bracket-style links as an additional parameter to set
>>  > faces on and not breaking existing hookups.
>>  
>>  Isn't the function called with point on the link? You may just need to
>>  extract the contents from the environment.
>>  
>>  Otherwise, a solution is to catch `wrong-number-of-arguments' error and
>>  call again the function with the old calling convention. See, e.g.,
>>  `org-link-open'.
>>  
>>  > To make this more palatable I would suggest it's changed so that the
>>  > 2nd argument will be a list of alists or keywords so to not have this
>>  > function signature problem if someone else comes up with a reason to
>>  > introduce yet more data.
>>  
>>  Beware the over-engineering. At this point, someone motivated enough can
>>  put an advice to the link fontification function.
>>  
>>  Regards,
>>  
>>  -- 
>>  Nicolas Goaziou
>>  
>>