Re: (Feature Request) have org-edit-special work inside non-environment LaTeX blocks, i.e. \( \) and \[ \]

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Timothy <tecosaur@gmail.com> writes:

> I had a look at that, to me this was cleaner than using multiple let
> bindings, like so
>
> (let ((context ...))
>   (unless  ... user error)
>   (let* ((contents ...)
>          (delim-length ...))
>    ...
>
> vs.
>
> (let* ((context ...)
>        (_ (unless ... user error))
>        (contents ...)
>        (delim-length ...))
>  ...
>        
> Personally I find the second one nicer. Thoughts?

Without hesitation, the first form is nicer. The second one is just
abusing let-binding. I die a little just by looking at it.

> I've changed to (string-match-p "\\`\\$[^$]" contents), as this seems
> like the idiomatic form, let me know if you're happy with this.

the "-p" part is not warranted here. In Emacs, every function is
expected to modify match data, unless specified in the docstring. Since
there is a cost in preserving match-data, we shouldn't do it without
a good reason. Here, there isn't.

Note that you'll find a zillion of places in Org code base that
contradict the above. But, hey, nobody's perfect.

> I'm not actually sure what's going on with your second suggested form,
> or why that may be better. If you'd mind explaining, that would be
> appriciated :)

See `rx' macro. S-exp regexps are usually easier to read (after an
initial struggle), and less likely to introduce subtle bugs. For long
regexps, this is important. For this one, either way is fine.

>> You could factor out (length contents) so it is only called once.
>
> I'm not sure if this a big deal, 

This is not a big deal, but poor practice, IMO.

> but I shoved it in the let* for now, let me know if that suffices.

Well. Ideally, let-binding should enclose the minimum part of the code
that needs the binding. Sometimes, an exception is tolerated, because
the code would contain too many nested let-forms. But, conversely, you
shouldn't stuff every local variable at the beginning of the function
and be done with it.

In this particular case, there's no reason to stuff the `length' call at
the top of the function when you need it later on, on a well-defined
S-exp. IOW, it is more idiomatic to just add a let-binding around the
appropriate (add-text-properties ...).

>>> +    (org-src--edit-element
>>> +     context
>>> +     (org-src--construct-edit-buffer-name (buffer-name) "LaTeX fragment")
>>> +     (org-src-get-lang-mode "latex")
>>> +     (lambda ()
>>> +       ;; Blank lines break things, replace with a single newline
>> 
>> See above.
>
> I'm not quite sure what I should see? I don't notice anything to factor
> out here.

It was just about the missing full stop. You looked at the moon, but
I really was the fool showing the tip of his finger ;)

>>> +       (while (re-search-forward "\n[ \t]*\n" nil t) (replace-match "\n"))
>>> +       ;; If within a table a newline would disrupt the structure,
>> 
>> This comment is truncated.
>
> Added ", so remove newlines"

But it should be ", so remove newlines."

> I recall being asked to list modified/added functions, what else do
> I need?

Nothing else.

>> Bonus points if you can add some tests in
>> "testing/lisp/test-org-src.el".
>
> I'll have a look at that, but I'm not quite sure what to do.

You could look at `test-org-src/footnote-references' for inspiration.
However, I assume tests will be less complicated for LaTeX fragments.

>> Could you remind me if you signed the FSF papers already?
>
> They're done and dusted :)

Perfect.

We're almost there, then.