Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Thank you. It looks fine, I will only be nitpicking.

Nitpick away :D

>> +(defun org-edit-latex-fragment ()
>> +  "Edit LaTeX fragment at point."
>> +  (interactive)
>> +  (let* ((context (org-element-context))
>> +	 (_ (unless (and (eq (org-element-type context) 'latex-fragment)
>> +			 (org-src--on-datum-p context))
>> +	      (user-error "Not on a LaTeX fragment")))
> 
> This is a fancy way to use a let-binding. I suggest to mimic what is
> done elsewhere, i.e., first bind `context', then check if we're at
> a LaTeX fragment, then bind the rest.

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?

>> +    ;; Grab the LaTeX fragment for propertization
> 
> Missing full stop at the end of the comment.

Fixed!

> 
>> +	 (contents (buffer-substring-no-properties
>> +		    (org-element-property :begin context)
>> +		    (- (org-element-property :end context)
>> +		       (org-element-property :post-blank context))))
>> +	 (delim-length (if (string-match "\\$[^$]" (substring contents 0 2))
> 
> Use 
> 
>    (string-match "\\`\\$[^$]" contents)
> 
> instead.
> 
> or, arguably better,
> 
>    (string-match (rx (seq string-start "$" (not (any "$")))) 
>                  contents)
> 
>> +			   1 2)))
>> +    ;; make the LaTeX deliminators read-only

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

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 :)
 
> Missing initial capital and final full stop.

Fixed!

> You could factor out (length contents) so it is only called once.

I'm not sure if this a big deal, but I shoved it in the let* for now,
let me know if that suffices.

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

> 
>> +       (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"

> Don't leave parenthesis alone.

Fixed!

> Also, make sure your indentation is right, e.g., using M-q on the
> definition.

I've applied auto-indent to `org-edit-latex-fragment'

> You also need to add a proper commit message and use `git format-patch',
> and an entry in ORG-NEWS (probably in Miscellaneous part).

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

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

> Could you remind me if you signed the FSF papers already?

They're done and dusted :)

Regards,

Timothy.