emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Nicolas Goaziou <mail@nicolasgoaziou.fr>
To: Timothy <tecosaur@gmail.com>
Cc: Bastien <bzg@gnu.org>, "emacs-orgmode@gnu.org" <emacs-orgmode@gnu.org>
Subject: Re: (Feature Request) have org-edit-special work inside non-environment LaTeX blocks, i.e. \( \) and \[ \]
Date: Tue, 26 May 2020 11:11:17 +0200	[thread overview]
Message-ID: <87mu5v6oy2.fsf@nicolasgoaziou.fr> (raw)
In-Reply-To: <BDF96401-83EB-4F79-BD8A-BADC0262FE06@getmailspring.com> (Timothy's message of "Tue, 26 May 2020 16:39:41 +0800")

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.


  reply	other threads:[~2020-05-26  9:11 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-19  1:31 (Feature Request) have org-edit-special work inside non-environment LaTeX blocks, i.e. \( \) and \[ \] Timothy
2020-05-19  7:47 ` Nicolas Goaziou
2020-05-19  9:27   ` Timothy
2020-05-19  9:39     ` Nicolas Goaziou
2020-05-19  9:45       ` Timothy
2020-05-19 13:27         ` Nicolas Goaziou
2020-05-19 13:32           ` Timothy
2020-05-19 14:09             ` Nicolas Goaziou
2020-05-19 14:12               ` Timothy
2020-05-19 14:28                 ` Nicolas Goaziou
2020-05-19 14:33                   ` Timothy
2020-05-23  9:10 ` Bastien
2020-05-23  9:20   ` tecosaur
2020-05-23  9:34     ` Bastien
2020-05-24 15:07   ` TEC
2020-05-24 15:38     ` TEC
2020-05-24 15:43       ` Timothy
2020-05-24 19:33         ` Nicolas Goaziou
2020-05-25  1:33           ` TEC
2020-05-25  7:11             ` Nicolas Goaziou
2020-05-25  9:28           ` TEC
2020-05-25  9:41             ` Nicolas Goaziou
2020-05-25  9:42               ` TEC
2020-05-25  9:55                 ` TEC
2020-05-25 10:09                   ` Nicolas Goaziou
2020-05-25 10:09                     ` TEC
2020-05-25 10:23                       ` Nicolas Goaziou
2020-05-25 10:24                         ` TEC
2020-05-25 10:32                           ` Nicolas Goaziou
2020-05-25 10:33                             ` TEC
2020-05-25 10:05                 ` TEC
2020-05-25 10:20                   ` Nicolas Goaziou
2020-05-25 10:21                     ` TEC
2020-05-25 11:27                       ` Nicolas Goaziou
2020-05-25 13:22                         ` Timothy
2020-05-25 14:08                           ` TEC
2020-05-26  7:56                             ` Nicolas Goaziou
2020-05-26  8:39                               ` Timothy
2020-05-26  9:11                                 ` Nicolas Goaziou [this message]
2020-05-26  9:23                                   ` TEC
2020-05-26 13:56                                     ` TEC
2020-05-26 21:13                                       ` Nicolas Goaziou
2020-05-26 17:01                                 ` John Kitchin
2020-05-25 18:58                           ` Nicolas Goaziou
2020-05-26  4:48                             ` TEC
2020-05-25 10:11                 ` Nicolas Goaziou
2020-05-25 10:17                   ` TEC
  -- strict thread matches above, loose matches on Subject: below --
2020-05-18  6:44 Timothy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87mu5v6oy2.fsf@nicolasgoaziou.fr \
    --to=mail@nicolasgoaziou.fr \
    --cc=bzg@gnu.org \
    --cc=emacs-orgmode@gnu.org \
    --cc=tecosaur@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).