Re: [ANN] lisp/ob-tangle-sync.el

[Ihor Radchenko]

Mehmet Tekman <mtekman89@gmail.com> writes:

>    It's a big patch mostly, because there were no intermediate commits
>    in which the org framework wouldn't be broken. Hope that's okay!

Sure it is.

> ** Problems
>
> It seems to work well for most tests, except for the "file with
> spaces.txt" which I'm not sure how to proceed with. I feel like
> patching at the level `org-babel--merge-params' is the wrong way to
> go, and that I should patch it further upstream.
>
> That upstream leads to `org-babel-read' which currently chomps the
> quotes off anything it gets:
>
> #+begin_src elisp
>   (org-babel-read " \"my file with quotes\" ")
> #+end_src

> I don't know if this is the expected behaviour for quoted strings with
> spaces, so before I proceed trying to patch this I thought I would
> check in with you and ask whether I should start patching here or not.

This is indeed a problem.
Org cannot distinguish between

#+begin_src lang :foo value with spaces
and
#+begin_src lang :foo "value with spaces"

What we can do it make `org-babel-read' assign a text property to the
resulting string when it is a string with quotes:

(org-babel-read "my file with quotes") ; => "my file with quotes"
(org-babel-read "\"my file with quotes\"") ; => #("my file with quotes" 0 19 
(org-babel-quote t))

Then, we can later use this information in `org-babel-merge-params'.
We will not call `split-string', when 'org-babel-quote text property is
present.
Also, `split-string' won't work when we have something like

"yes \"my file with quotes\""

Instead, we should use
(mapcar #'org-babel-read (org-babel-balanced-split "yes \"my file with 
quotes\"" ?\s))

> -    (tangle  . ((tangle yes no :any)))
> +    (tangle  . ((tangle yes no :any)
> +                  (import export skip sync)))

We should not yet change this before the code using the new tangle
values is in place.

> +(defun org-babel--merge-headers (exclusive-groups &rest result-params)
> +  "Maintain exclusivity of mutually exclusive parameters, as defined
> +in EXCLUSIVE-GROUPS while merging lists in RESULT-PARAMS."

Please update the docstring, following conventions:
1. First line should be a short full sentence explaining what function
does briefly.
2. Each argument should be explained - what it is and what is the
   structure.

Also, RESULT-PARAMS is confusing because it is no longer just used for
:results. Please rename.

> +  (cl-flet ((alist-append (alist key val)
> +              (let ((existing (cdr (assoc key alist))))
> +                (if existing
> +                    (setcdr (assoc key alist) (append existing (list val)))
> +                  (setq alist (cons (cons key (list val)) alist)))
> +                alist)))

Can simply
(push val (alist-get key alist nil nil #'equal))

> +    ;; Problem: Having an :any keyword in an exclusion group means
> +    ;;          that a parameter of "yes" could match to an exclusion
> +    ;;          group that contains both "yes" AND ":any".
> +    ;; Solution: For each parameter, build a list of exclusion groups
> +    ;;           it could belong to. If a parameter belongs to two
> +    ;;           groups, eliminate it from the group that contains the
> +    ;;           ":any" keyword.
> +    ;;
> +    ;;  Details: Exclusion groups (e.g.'("tangle" "yes" "no" ":any") )
> +    ;;           are referenced to by their car ("tangle"), acting as
> +    ;;           a key for the entire group.
> +    ;;

This is not a "problem" - just something to consider. You can omit
"Problem:", "Solution:", and "Details:" converting into ordinary
paragraphs.

> +    ;; Assumption: The order of RESULT-PARAMS dictate the hierarchy of
> +    ;;             the cascading headers.

The expected function parameters should be described in the docstring,
not in a comment.

> +    (let ((any-group-key       ;; exclusion group key for group with :any 
> keyword
> +           (caar (cl-remove-if-not (lambda (x) (member ":any" x)) 
> exclusive-groups)))

`cl-find' will be simpler.

> +      (delete-dups unexplained-params)

Should be
(setq unexplained-params (delete-dups unexplained-params))

`delete-dups' _returns_ the modified list, which may or may not preserve
the initial pointer to the first cons cell.

> +        ;; Set values in the same order that the exclusion groups are defined
> +        (let ((group-key-order (mapcar #'car exclusive-groups)))
> +          (setq group-alist (cl-remove-if-not #'identity

Or just delq nil

> +                                              (mapcar (lambda (x) (car 
> (alist-get x group-alist)))
> +                                                      group-key-order))))

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <http://orgmode.org/>.
Support Org development at <http://liberapay.com/org-mode>,
or support my work at <http://liberapay.com/yantar92>