Timothy <tecos...@gmail.com> writes:
> Nicolas Goaziou <m...@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"))) Is this a case for the if-let* macro? Or is that too new a feature for org to use still? >> >> 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. -- Professor John Kitchin Doherty Hall A207F Department of Chemical Engineering Carnegie Mellon University Pittsburgh, PA 15213 412-268-7803 @johnkitchin http://kitchingroup.cheme.cmu.edu