[PATCH] Remove additional newline at end of results block

[PATCH] Remove additional newline at end of results block

[Matt Huszagh]

[-- Attachment #1: Type: text/plain, Size: 2900 bytes --]

Inserting this newline prevents a valid use-case and protects against
an edge-case that is completely avoidable without the additional
guarantee it provides. The original intention for inserting the
newline was to avoid the edge case in which a user does not insert a
newline between a source block and the subsequent text and the
subsequent text is merged with the results.

However, there are valid cases in which a user would not want a
newline between a results block and the subsequent buffer text. For
example, many display equations in LaTeX are considered as part of the
surrounding paragraph. Additionally, it is possible to setup a LaTeX
source block to be executable and insert results into the org
buffer. Consider the following example:

some org file (leading colons to prevent git ignoring lines starting
with #):
```
: We can write the simplest equation as
: #+begin_src latex
: \begin{equation}
:   1 + 1 = 2,
: \end{equation}
: #+end_src
: and hope that no one is confused by this.
```

We might then execute this source block to generate some output. For
example, this might generate and SVG image of the block and insert it
into the buffer. That should result in something like

```
: We can write the simplest equation as
: #+begin_src latex
: \begin{equation}
:   1 + 1 = 2,
: \end{equation}
: #+end_src

: #+RESULTS:
: #+begin_results
: [[file:some/file.svg]]
: #+end_results
: and hope that no one is confused by this.
```

When formatted this way, the resulting tex
file (org-latex-export-to-latex) will be:

```
We can write the simplest equation as
\begin{equation}
  1 + 1 = 2,
\end{equation}
and hope that no one is confused by this.
```

which will render correctly. Specifically, the display math
environment will not start a new paragraph after the leading "as" and
a new paragraph will not start between the end of the math display and
the trailing "and".

However, the current behavior results in

```
: We can write the simplest equation as
: #+begin_src latex
: \begin{equation}
:   1 + 1 = 2,
: \end{equation}
: #+end_src

: #+RESULTS:
: #+begin_results
: [[file:some/file.svg]]
: #+end_results

: and hope that no one is confused by this.
```

This blank line necessarily starts a new paragraph in TeX.

Finally, as previously stated, it is entirely possible to control
whether there is a newline between the results block and subsequent
text by leaving a newline between the source block and text. For
example,

```
: We can write the simplest equation as
: #+begin_src latex
: \begin{equation}
:   1 + 1 = 2,
: \end{equation}
: #+end_src

: and hope that no one is confused by this.
```

would still result in

```
: We can write the simplest equation as
: #+begin_src latex
: \begin{equation}
:   1 + 1 = 2,
: \end{equation}
: #+end_src

: #+RESULTS:
: #+begin_results
: [[file:some/file.svg]]
: #+end_results

: and hope that no one is confused by this.
```

[-- Attachment #2: 0001-lisp-ob-core.el-Remove-additional-newline-at-end-of-.patch --]
[-- Type: text/x-patch, Size: 4138 bytes --]

From 667ba67570361bad878c74afdebe068b7916a1bb Mon Sep 17 00:00:00 2001
From: Matt Huszagh <huszaghmatt@gmail.com>
Date: Thu, 30 Dec 2021 06:54:24 -0800
Subject: [PATCH] lisp/ob-core.el: Remove additional newline at end of results

* lisp/ob-core.el (org-babel--insert-results-keyword): Remove newline
at end of results block.

Inserting this newline prevents a valid use-case and protects against
an edge-case that is completely avoidable without the additional
guarantee it provides. The original intention for inserting the
newline was to avoid the edge case in which a user does not insert a
newline between a source block and the subsequent text and the
subsequent text is merged with the results.

However, there are valid cases in which a user would not want a
newline between a results block and the subsequent buffer text. For
example, many display equations in LaTeX are considered as part of the
surrounding paragraph. Additionally, it is possible to setup a LaTeX
source block to be executable and insert results into the org
buffer. Consider the following example:

some org file (leading colons to prevent git ignoring lines starting
with #):
```
: We can write the simplest equation as
: #+begin_src latex
: \begin{equation}
:   1 + 1 = 2,
: \end{equation}
: #+end_src
: and hope that no one is confused by this.
```

We might then execute this source block to generate some output. For
example, this might generate and SVG image of the block and insert it
into the buffer. That should result in something like

```
: We can write the simplest equation as
: #+begin_src latex
: \begin{equation}
:   1 + 1 = 2,
: \end{equation}
: #+end_src

: #+RESULTS:
: #+begin_results
: [[file:some/file.svg]]
: #+end_results
: and hope that no one is confused by this.
```

When formatted this way, the resulting tex
file (org-latex-export-to-latex) will be:

```
We can write the simplest equation as
\begin{equation}
  1 + 1 = 2,
\end{equation}
and hope that no one is confused by this.
```

which will render correctly. Specifically, the display math
environment will not start a new paragraph after the leading "as" and
a new paragraph will not start between the end of the math display and
the trailing "and".

However, the current behavior results in

```
: We can write the simplest equation as
: #+begin_src latex
: \begin{equation}
:   1 + 1 = 2,
: \end{equation}
: #+end_src

: #+RESULTS:
: #+begin_results
: [[file:some/file.svg]]
: #+end_results

: and hope that no one is confused by this.
```

This blank line necessarily starts a new paragraph in TeX.

Finally, as previously stated, it is entirely possible to control
whether there is a newline between the results block and subsequent
text by leaving a newline between the source block and text. For
example,

```
: We can write the simplest equation as
: #+begin_src latex
: \begin{equation}
:   1 + 1 = 2,
: \end{equation}
: #+end_src

: and hope that no one is confused by this.
```

would still result in

```
: We can write the simplest equation as
: #+begin_src latex
: \begin{equation}
:   1 + 1 = 2,
: \end{equation}
: #+end_src

: #+RESULTS:
: #+begin_results
: [[file:some/file.svg]]
: #+end_results

: and hope that no one is confused by this.
```
---
 lisp/ob-core.el | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/lisp/ob-core.el b/lisp/ob-core.el
index 7a9467b0e..c0cb283bd 100644
--- a/lisp/ob-core.el
+++ b/lisp/ob-core.el
@@ -1994,14 +1994,9 @@ the results hash, or nil.  Leave point before the keyword."
 		  ":"
 		  (when name (concat " " name))
 		  "\n"))
-  ;; Make sure results are going to be followed by at least one blank
-  ;; line so they do not get merged with the next element, e.g.,
-  ;;
-  ;;   #+results:
-  ;;   : 1
-  ;;
-  ;;   : fixed-width area, unrelated to the above.
-  (unless (looking-at "^[ \t]*$") (save-excursion (insert "\n")))
+  ;; We deliberately do not insert a newline here since there are
+  ;; valid cases in which a user does not want a blank line between a
+  ;; results block and the subsequent text.
   (beginning-of-line 0)
   (when hash (org-babel-hide-hash)))
 
-- 
2.31.1

Re: [PATCH] Remove additional newline at end of results block

[Ihor Radchenko]

Matt Huszagh <huszaghmatt@gmail.com> writes:

> +  ;; We deliberately do not insert a newline here since there are
> +  ;; valid cases in which a user does not want a blank line between a
> +  ;; results block and the subsequent text.
>    (beginning-of-line 0)
>    (when hash (org-babel-hide-hash)))

Thanks for the patch! And sorry for the late reply.

Your explanation is quite confusing, but I think I managed to understand
what you referred to:

------
 We can write the simplest equation as
 #+begin_src latex :exports results
 \begin{equation}
   1 + 1 = 2,
 \end{equation}
 #+end_src
the paragraph should continue here.
--------

will be exported as

------
We can write the simplest equation as
\begin{equation}
  1 + 1 = 2,
\end{equation}

the paragraph should continue here.
------

Note the extra empty line below equation environment.

However, I am not confident if the proposed change is going to be safe
for other uses of src blocks.

I'd like to request other people who use export and source blocks
extensively to try the patch and see if it breaks anything.

Meanwhile, could you please reword the commit message and make it more
concise and clear?

Best,
Ihor

Re: [PATCH] Remove additional newline at end of results block

[Matt Huszagh]

Ihor Radchenko <yantar92@gmail.com> writes:

> Thanks for the patch! And sorry for the late reply.

No worries for the delay - I know you have a lot of these. Thanks for
the review.

> Your explanation is quite confusing, but I think I managed to understand
> what you referred to:
>
> ------
>  We can write the simplest equation as
>  #+begin_src latex :exports results
>  \begin{equation}
>    1 + 1 = 2,
>  \end{equation}
>  #+end_src
> the paragraph should continue here.
> --------
>
> will be exported as
>
> ------
> We can write the simplest equation as
> \begin{equation}
>   1 + 1 = 2,
> \end{equation}
>
> the paragraph should continue here.
> ------
>
> Note the extra empty line below equation environment.
>
> However, I am not confident if the proposed change is going to be safe
> for other uses of src blocks.
>
> I'd like to request other people who use export and source blocks
> extensively to try the patch and see if it breaks anything.
>
> Meanwhile, could you please reword the commit message and make it more
> concise and clear?

Can you clarify what you find to be unclear? Rereading my own commit
message, my only problem with it is how it starts: I'd add one sentence
to contextualize it a bit. For instance,

The previous behavior always ensured the presence of an empty line
between the results block of a source block and the subsequent
text. However, inserting this newline prevents a valid use-case and
protects against an edge-case that is completely avoidable without the
additional guarantee it provides. ... (the rest remains unchanged)

Oh and I also made a typo. A sentence further down should read
"...generate an SVG image...".

This commit message isn't short, but I think it's very clear. It
describes the previous behavior, explains the rationale for that
behavior, and then illustrates (with a complete example) how this
prevents a valid use case. It also explains why the new change does not
prohibit any behavior that was previously possible.

As someone who frequently uses git log, I'd much rather see a commit
message like this than the typical (usually) unhelpful one or two
sentences that fail to provide the motivation for a change. There's no
downside to a long commit message, and this one is structured such that
it proceeds from more general to more specific information - not
everyone has to read the entire thing.

I'm not trying to be difficult :) but I do care about the quality of
this codebase (as I know do you), so I'm reluctant to change something
in a way I feel is inferior. But, if you have specific parts etc you
feel are unclear I'm more than happy to address those.

Matt

Re: [PATCH] Remove additional newline at end of results block

[Ihor Radchenko]

I am replying here because the original reply did not go into my inbox.

Matt Huszagh <huszaghmatt@gmail.com> writes:

>> I'd like to request other people who use export and source blocks
>> extensively to try the patch and see if it breaks anything.
>>
>> Meanwhile, could you please reword the commit message and make it more
>> concise and clear?
>
> Can you clarify what you find to be unclear? Rereading my own commit
> message, my only problem with it is how it starts: I'd add one sentence
> to contextualize it a bit. For instance,

I will provide a detailed response below.

Also, I have found an edge case where your patch creates an issue:

#+begin_src emacs-lisp
2
#+end_src
: fixed-width area, unrelated to the above.

If you execute the above code with your patch, the fixed width area gets
deleted.

> The previous behavior always ensured the presence of an empty line
> between the results block of a source block and the subsequent
> text. However, inserting this newline prevents a valid use-case and
> protects against an edge-case that is completely avoidable without the
> additional guarantee it provides. ... (the rest remains unchanged)

The second sentence is hard to understand because (1) it contains two
non-obvious statements; (2) the statements will only become clear for
the reader after reading the next sentences. In writing, it is generally
advised to start from something reader is familiar with and introduce
new things one by one.

> This commit message isn't short, but I think it's very clear. It
> describes the previous behavior, explains the rationale for that
> behavior, and then illustrates (with a complete example) how this
> prevents a valid use case. It also explains why the new change does not
> prohibit any behavior that was previously possible.
>
> As someone who frequently uses git log, I'd much rather see a commit
> message like this than the typical (usually) unhelpful one or two
> sentences that fail to provide the motivation for a change. There's no
> downside to a long commit message, and this one is structured such that
> it proceeds from more general to more specific information - not
> everyone has to read the entire thing.
>
> I'm not trying to be difficult :) but I do care about the quality of
> this codebase (as I know do you), so I'm reluctant to change something
> in a way I feel is inferior. But, if you have specific parts etc you
> feel are unclear I'm more than happy to address those.

Sorry, I think you misunderstood my intentions. I am not against
detailed commit messages. I also prefer when commits have sufficient
information to understand the motivation behind.

However, there is no reason to give lengthy and hard-to-understand
explanations when more concise wording is possible.

Now, detailed comments on what is confusing about the commit message:

> * lisp/ob-core.el (org-babel--insert-results-keyword): Remove newline
> at end of results block.

This is minor, but I'd rather say "Do not add newline ...". Because it
is what the patch actually does.

> Inserting this newline prevents a valid use-case and protects against
> an edge-case that is completely avoidable without the additional
> guarantee it provides. The original intention for inserting the
> newline was to avoid the edge case in which a user does not insert a
> newline between a source block and the subsequent text and the
> subsequent text is merged with the results.

This is hard to understand.

It is unclear which part of code you are referring to. If a reader is
not familiar with the changed function, the first statement "inserting
this newline..." is unclear. What does "this" refer to?

In the second sentence you are trying to describe the original edge
case, but it is really hard to imagine without having an example.

I'd say something like:

"Previously, `org-babel--insert-results-keyword' inserted an empty line
after result of evaluation even when the original source block had no
empty lines.  This was done to address the following issue:"

Then, I'd provide an example on the original issue.

Then, I'd continue with your original wording:

> However, there are valid cases in which a user would not want a
> newline between a results block and the subsequent buffer text. For
> example, many display equations in LaTeX are considered as part of the
> surrounding paragraph. Additionally, it is possible to setup a LaTeX
> source block to be executable and insert results into the org
> buffer. Consider the following example:
>
> some org file (leading colons to prevent git ignoring lines starting
> with #):
> ```
> : We can write the simplest equation as
> : #+begin_src latex
> : \begin{equation}
> :   1 + 1 = 2,
> : \end{equation}
> : #+end_src
> : and hope that no one is confused by this.
> ```

"and hope that no one is confused by this" sounds too similar to the
main commit message. When I was reading your example Org text, I kept
confusing this phrase with the actual message.

> We might then execute this source block to generate some output. For
> example, this might generate and SVG image of the block and insert it
> into the buffer. That should result in something like
>
> ```
> : We can write the simplest equation as
> : #+begin_src latex
> : \begin{equation}
> :   1 + 1 = 2,
> : \end{equation}
> : #+end_src
>
> : #+RESULTS:
> : #+begin_results
> : [[file:some/file.svg]]
> : #+end_results
> : and hope that no one is confused by this.
> ```

This is confusing because image is _not_ produced by ob-latex.el with
default settings. Moreover, attempting to export will _not_ export to
:exports both. The default value is :exports results.

Are you talking about export here? Something else? Where is the newline
you were talking about?

> When formatted this way, the resulting tex
> file (org-latex-export-to-latex) will be:
>
> ```
> We can write the simplest equation as
> \begin{equation}
>   1 + 1 = 2,
> \end{equation}
> and hope that no one is confused by this.
> ```

> which will render correctly. Specifically, the display math
> environment will not start a new paragraph after the leading "as" and
> a new paragraph will not start between the end of the math display and
> the trailing "and".

This is confusing because you introduce the behavior after the patch
before showing what is wrong with the current behavior.

I was reading the above as the current behavior until I reached the next
paragraph. Then, I felt lost.

> However, the current behavior results in
>
> ```
> : We can write the simplest equation as
> : #+begin_src latex
> : \begin{equation}
> :   1 + 1 = 2,
> : \end{equation}
> : #+end_src
>
> : #+RESULTS:
> : #+begin_results
> : [[file:some/file.svg]]
> : #+end_results
>
> : and hope that no one is confused by this.
> ```
>
> This blank line necessarily starts a new paragraph in TeX.

This is again confusing. You just showed export output that did not
generate the image and now you suddenly do show an image.

Note that I do understand that you structured the logic as: (1) show
expected result of C-c C-c; (2) show expected result of export; (3) show
the current wrong result of C-c C-c; (4) show the current wrong result
of export. However, it is not the logic I expected. I only managed to
grasp it after reading the whole commit message multiple times.

> Finally, as previously stated, it is entirely possible to control
> whether there is a newline between the results block and subsequent
> text by leaving a newline between the source block and text. For
> example,
>
> ```
> : We can write the simplest equation as
> : #+begin_src latex
> : \begin{equation}
> :   1 + 1 = 2,
> : \end{equation}
> : #+end_src
>
> : and hope that no one is confused by this.
> ```
>
> would still result in
>
> ```
> : We can write the simplest equation as
> : #+begin_src latex
> : \begin{equation}
> :   1 + 1 = 2,
> : \end{equation}
> : #+end_src
>
> : #+RESULTS:
> : #+begin_results
> : [[file:some/file.svg]]
> : #+end_results
>
> : and hope that no one is confused by this.
> ```

And now my item (4) in the above understanding is not followed. I do not
see where this example is coming from. Current wrong behavior? On
export? On C-c C-c? After patch? Confusing.

I would drop the C-c C-c examples and just focus on export, what is
wrong with current export behavior and what the patch will improve.

Best,
Ihor