Re: [PATCH] org-protocol: Allow key=val&key2=value2-style URLs

[Aaron Ecay <aaronecay@gmail.com>]

Hi Sacha,

Thanks for the patch.  It looks great!

I assume eventually we will want to deprecate the old-style links, and
make new-style the only style.  Unfortunately, this would mean another
API change to remove the ‘new-style’ arguments from these functions.
I don’t have any clever ideas to solve this, and it’s not an objection
to your patch – just something I thought I’d mention in case someone can
think of a way to deal with the eventual deprecation without an API
change.

A few comments below:

2015ko abenudak 4an, Sacha Chua-ek idatzi zuen:

> From aff151930a73c22bb3fdf3ae9b442cecc08aaa67 Mon Sep 17 00:00:00 2001
> From: Sacha Chua <sacha@sachachua.com>
> Date: Wed, 2 Dec 2015 10:53:07 -0500
> Subject: [PATCH] org-protocol: Allow key=val&key2=val2-style URLs
> 
> * lisp/org-protocol.el: Update documentation.
>   (org-protocol-parse-parameters): New function to simplify handling of
>   old- or new-style links.
>   (org-protocol-assign-parameters): New function to simplify handling of
>   old- or new-style links.

You can combine these like:

(org-protocol-parse-parameters, org-protocol-assign-parameters): New
functions.

I also think the convention in Changelogs is not to put in details, but
just to say “New function” or “Accept new-style links”.  A narrative
explanation can be put in the git commit message below the changelog
section (and will not be included in the Changelog file distributed with
Emacs).  But I’ll admit I don’t understand Changelog conventions and
think they are a pointless relic, so YMMV.

[...]

>  
> +(defun org-protocol-parse-parameters (info new-style &optional default-order unhexify separator)

Is there ever a case where we would want unhexify to be something other
than t?  Hexification is imposed by the URL format, there is no optionality
about it.  Handler functions get access to the raw string if they need it
for some reason, I don’t think our helper functions need to bother with the
unhexify != t case.  Similarly, I would not have a separator argument, but
use the value of ‘org-protocol-data-separator’ directly.  In the rare case
that a caller needs to influence the separator, they can let-bind that
variable.

TLDR: can we get rid of unhexify and separator arguments?

[...]
  
>  (defun org-protocol-check-filename-for-protocol (fname restoffiles client)
>    [...docstring omitted...]
>    (let ((sub-protocols (append org-protocol-protocol-alist
>  			       org-protocol-protocol-alist-default)))
>      (catch 'fname
> @@ -532,19 +604,27 @@ as filename."
>          (when (string-match the-protocol fname)
>            (dolist (prolist sub-protocols)
>              (let ((proto (concat the-protocol
> -				 (regexp-quote (plist-get (cdr prolist) :protocol)) ":/+")))
> +				 (regexp-quote (plist-get (cdr prolist) :protocol)) "\\(:/+\\|\\?\\)")))
>                (when (string-match proto fname)
>                  (let* ((func (plist-get (cdr prolist) :function))
>                         (greedy (plist-get (cdr prolist) :greedy))
>                         (split (split-string fname proto))
> -                       (result (if greedy restoffiles (cadr split))))
> +                       (result (if greedy restoffiles (cadr split)))
> +		       (new-style (string= (match-string 1 fname) "?")))

As a way to encourage users to move to new-style links, should we add a
warning if new-style = nil?

Thanks again for the patch,

-- 
Aaron Ecay