Hi,

Thanks for the feedback.

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

>> +(defun org-latex-environment--type (latex-environment)
>
> It should be `org-latex--environment-type'.

Yes.

> I'd rather avoid using `org-latex-regexps', which predates the parser.
> A hard-coded regexp is better.

OK.


>> +	 (env (progn (string-match latex-begin-re value)
>> +		     (match-string 2 value))))
>
> Since environments do not necessary start with \begin{...}, I think the
> following is better
>
>   (and (string-match ...)
>        (match-string ...))

Don't the element `latex-environment' always start \begin{.}?
Cf. org-element--latex-begin-environment.

Another danger is that someone writes something like,

    \begin{center}
    \begin{table}
    ...

I wouldn’t worry too much about this, though.

>> +    (cond
>> +     ((string-match org-latex-math-environments-re value) 'math)
>> +     ((string-match-p "tab\\(le\\|ular\\)" env) 'table)
>
> This is a bit sloppy. In particular, it doesn't match all table
> environments supported out of the box, e.g., "longtabu". Also, a list of
> strings, compiler into a regexp with `regexp-opt' may be better.

Longtable in an omission, but would have been matched.  For some reason I
thought tabu was no longer supported.  Browsing the tabu CTAN page there’s
links to all sorts of exotic table packages, like "stabular",
"bigtabular", and "supertabular".  I wonder if it’s enough to match the
ones supported by Org by default, or whether it would be better to have it
as a defvar that could be hacked if necessary?

>> +	;; is not a math environment.
>>  	(with-temp-buffer
>>  	  (insert value)
>> -	  (goto-char (point-min))
>> -	  (forward-line)
>> -	  (insert (org-latex--label latex-environment info nil t))
>> +	  (if caption-above-p
>> +	      (progn
>> +		(goto-char (point-min))
>> +		(forward-line)
>> +		(insert caption))
>> +	    (goto-char (point-max))
>> +	    (forward-line -1)
>> +	    (insert caption))
>
> Nitpick: you can move (insert caption) outside the (if ...) and
> de-duplicate it.

Good point.  Thanks.

I have attached the patch with the changes again and added a changelog
entry.  I am not sure this needs to be documented, but I’m happy to
include a couple lines somewhere.

Thanks,
Rasmus

-- 
It was you, Jezebel, it was you