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

[Ihor Radchenko <yantar92@gmail.com>]

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