Re: [PATCH] org-babel-demarcate-block: split using element API

[gerard . vermeulen]

On 04.01.2024 15:43, Ihor Radchenko wrote:
> gerard.vermeulen@posteo.net writes:
Attached you'll find a new version of the patch that addresses your
comments.  I have modified the ERT test so that it checks most of
your examples showing where the older versions of the patch failed.
The test is now called `test-ob/demarcate-block'

It also allows to split in three blocks when a region is selected (main
does this contrary to my older patches).

Below, I compare region splitting using main or my patch.  White-space
differs between main and the patch and one might argue that the result
produced by the patch is more consistent.  Maybe, the indenting of the
input code block is somewhat contrived, because all code is moved
completely to the left after calling `org-indent-block'.

* main does this
#+begin_src emacs-lisp :results silent
  (setopt org-adapt-indentation t
          org-src-preserve-indentation t
          org-edit-src-content-indentation 2)
#+end_src
******** before C-u org-babel-demarcate-block region splitting
         #+begin_src emacs-lisp
           (defun above ()
             (message "above"))

           (defun region ()
             (message "mark region with leading and trailing blank 
lines"))

           (defun below ()
             (message "below"))
         #+end_src
******** after C-u org-babel-demarcate-block region splitting
         #+begin_src emacs-lisp
           (defun above ()
             (message "above"))
#+end_src
********
#+begin_src emacs-lisp
           (defun region ()
             (message "mark region with leading and trailing blank 
lines"))

           #+end_src
********
           #+begin_src emacs-lisp
           (defun below ()
             (message "below"))
         #+end_src
* end main does this

* patch does this
#+begin_src emacs-lisp :results silent
  (setopt org-adapt-indentation t
          org-src-preserve-indentation t
          org-edit-src-content-indentation 2)
#+end_src
******** before C-u org-babel-demarcate-block region splitting
         #+begin_src emacs-lisp
           (defun above ()
             (message "above"))

           (defun region ()
             (message "mark region with leading and trailing blank 
lines"))

           (defun below ()
             (message "below"))
         #+end_src
******** after C-u org-babel-demarcate-block region splitting
         #+begin_src emacs-lisp
(defun above ()
  (message "above"))
         #+end_src
********
         #+begin_src emacs-lisp

(defun region ()
  (message "mark region with leading and trailing blank lines"))
         #+end_src
********
         #+begin_src emacs-lisp
(defun below ()
  (message "below"))
         #+end_src
* end patch does this

> > I have tried to clean up the code.  I have also tried to get 
> > `body-beg'
> > and
> > `body-end' marking the text between the #+begin_src and #+end_src 
> > lines
> > from the element API, but I failed and had to fall back to
> > `org-babel-where-is-src-block-head'.  But only for that.
>
> org-element API does not provide this information for now. Maybe it is 
> a
> good opportunity to alter the parser, so that code boundaries are
> provided...
>
> >  (defun org-babel-demarcate-block (&optional arg)
> > ...
> > -When called within blank lines after a code block, create a new code
> > -block of the same language with the previous."
>
> Is there any reason why you dropped this feature?
>
> When I try
>
> #+begin_src emacs-lisp
> (+ 1 2)
> #+end_src
> <point>
>
> M-x org-babel-demarcate-block throws an error with your patch.
> It creates a new block with the same language before your patch.

Agreed, this is wrong. A partial explanation is that I attached too
much value to the doc-string of `org-babel-get-src-block-info'
telling "Return nil if point is not on a source block.  Otherwise," 
which
is for me in contradiction with documentation (string and start
comment) in `org-babel-demarcate-block'.

I have patched the doc-string of `org-babel-get-src-block-info' to
add the "blank lines below condition".

This patch reverts all changes that are due to my misunderstanding
of what `org-babel-get-src-block-info' does.

Now demarcating with point below a source block works again and
checking this is part of the ERT test.
> > +  (let ((copy (org-element-copy (org-element-at-point)))
> > +        (stars (concat (make-string (or (org-current-level) 1) ?*) " 
> > ")))
> > +    (if (eq 'src-block (car copy))
>
> You can instead use `org-element-type-p'
This is now back to the original (if (and info start) ;; At src block, 
but ...
> > +        ;; Keep this branch in sync with 
> > test-ob/demarcate-block-split.
> > +        ;; _start is never nil, since there is a source block element 
> > at point.
>
> May you elaborate what you mean by "keep in sync"?
"keep in sync" is a kind of reminder to myself, because I think that
test-ob/demarcate-block-split was fragile wrt where point is after
demarcation.

The test is now called test-ob/demarcate-block and I tried to make
it more robust.
> > +        (let* ((_start (org-babel-where-is-src-block-head))
>
> Are you using (org-babel-where-is-src-block-head) for side effect of
> modifying the match data? If so, please do it outside let, with
> appropriate comment.
This was based on my misunderstanding of `org-babel-get-src-block-info'
and has been removed.
> > +          (if (not org-adapt-indentation)
> > +              ;; Move point to the left of the lower block line 
> > #+begin_src.
> > +              (org-previous-block 1)
> > +            ;; Adapt the indentation: upper block first and lower 
> > block second.
> > +            (org-previous-block 2)
> > +            (org-indent-block)
> > +            ;; Move point to the left of the lower block line 
> > #+begin_src.
> > +            (org-next-block 1)
> > +            (org-indent-block)))
>
> `org-indent-block' should honor `org-adapt-indentation'. You do not 
> need
> to call it conditionally. Re-indenting unconditionally should be better
> here.
OK. I have always used `org-adapt-indentation' set to nil and I do not 
like
the result of `org-indent-block' when it is non-nil (#+begin_src and 
#+end_src
indented and the code pushed to the left), but I will have to get used 
to it.

Tests using `org-adapt-indention'  non-nil are part of 
`test-ob/demarcate-block'.
> >        (let ((start (point))
> > -           (lang (or (car info) ; Reuse language from previous block.
> > -                      (completing-read
> > -                      "Lang: "
> > -                      (mapcar #'symbol-name
> > -                              (delete-dups
> > -                               (append (mapcar #'car org-babel-load-languages)
> > -                                       (mapcar (lambda (el) (intern (car el)))
> > -                                               org-src-lang-modes)))))))
> > +            ;; (org-babel-get-src-block-info 'no-eval) returns nil,
> > +            ;; since there is no source block at point.  Therefore, 
> > this
> > +            ;; cannot be used to get the language of a neighbour 
> > block.
>
> Why nil? The condition was
>
>   (and info start) ;; At src block, but not within blank lines after 
> it.
>
> So, this branch of the if used to be INFO - non-nil, and START nil ->
> re-use the information. And if INFO were nil, query.
This was based on my misunderstanding of `org-babel-get-src-block-info'
and has been reverted.
> > +            ;; Deleted code indicated that this may have worked in 
> > the past.
> > +            ;; I have removed upper-case-p, since it could never be 
> > true here.
>
> The idea of UPPER-CASE-P is to keep user preference for keyword style
> (upper case or lower case). There is no reason to remove this feature.
> Although, since we are using `org-element-interpret-data', it might be 
> a
> good idea to extend org-element parser to preserve the keyword case
> information.
This was based on my misunderstanding of `org-babel-get-src-block-info'
and has been removed.

Regards -- Gerard

0001-org-babel-demarcate-block-split-using-element-API.patch
Description: Binary data