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

[Ihor Radchenko]

gerard.vermeulen@posteo.net writes:

> 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'

Thanks!
I've tested your patch and found two problems:

1. #+name: lines are duplicated, while they should not
2. Your patch does not create space between the src blocks, unlike what
   we have on main.
   I think that you need to (1) create a single blank lines between
   blocks (set :post-blank property to 1); (2) keep the number blank
   lines after the last block the same as in the initial block (copy the
   :post-blank property and assign it to the last inserted src block).

   For C-u argument, do not do anything special - just keep the original
   :post-blank as is. It is the closest to what we have on main.

> 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.

Agree.

> 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'.

`org-babel-get-src-block-info' docstring were not wrong. You just missed
the Org mode's convention that blank lines after src blocks or other
syntax elements belong to these elements.

That said, we may clarify the `org-babel-get-src-block-info' docstring
to highlight this fact and avoid future confusion.

> Now demarcating with point below a source block works again and
> checking this is part of the ERT test.

The ERT test does not check removing #+caption from the original block.
Also, as I said above, we should remove #+name.

>> `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.

Note that indentation of src blocks has been refactored recently on
main. It should be more reliable now. If not, please report any issues.

> -;; Copyright (C) 2009-2024 Free Software Foundation, Inc.
> +;; Copyright (C) 2009-2023 Free Software Foundation, Inc.

This is a spurious change :)
  
> -Return nil if point is not on a source block.  Otherwise, return
> -a list with the following pattern:
> +Return nil if point is not on a source block or not within blank
> +lines after a source block.  Otherwise, return a list with the
> +following pattern:

I'd rather say: Return nil if point is not on a source block (blank
lines after a source block are considered a part of that source block).

It would be more accurate.
  
> +        (let* ((copy (org-element-copy (org-element-at-point)))
> +               (before (org-element-begin copy))
> +               (beyond (org-element-end copy))
> +               (parts (sort (if (org-region-active-p)
> +                                (list body-beg (mark) (point) body-end)
> +                              (list body-beg (point) body-end))
> +                            #'<)))
> +          ;; Prevent #+caption:, #+header:, and #+begin_src lines in block.

This comment is out of place. Also, we do preserve #+header and
#+begin_src lines, don't we?

And we need to remove #+name lines.

> +          (unless (and (>= (point) body-beg))
> +            (user-error "move point within the source block body to split 
> it"))

Please start error message from capital letter. It is Elisp convention
(see `user-error' docstring).

> +          (setq parts (mapcar (lambda (p) (buffer-substring (car p) (cdr p)))
> +                              (seq-mapn #'cons parts (cdr parts))))
> +          (delete-region before beyond)
> +          (deactivate-mark)

AFAIK, `deactivate-mark' should be unnecessary here. To indicate that
region should be deactivated after finishing a command, we simply set
`deactivate-mark' _variable_ to t. See the docstring.

-- 
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>