Mehmet Tekman <mtekma...@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 <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
