emacs-orgmode
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [O] Add preamble support to ob-plantuml.el


From: Thibault Marin
Subject: Re: [O] Add preamble support to ob-plantuml.el
Date: Fri, 09 Dec 2016 22:48:39 -0600
User-agent: mu4e 0.9.16; emacs 24.5.1

Hi,

Nicolas Goaziou writes:
> Comments follow.
>
>> +(defun org-babel-plantuml-var-to-plantuml (var)
>> +  "Cleanup plantuml variable (remove quotes)."
>> +     (replace-regexp-in-string "\"" "" var))
>
> Since this function is used only once in the code, I suggest to not
> implement it and use `replace-regexp-in-string' at the appropriate
> place.
I was trying to match what other ob-*s do.  If the table assignment idea
was to be implemented (for instance), using a separate function may be
cleaner.  But the function is indeed currently not needed, so I removed
it.

>> +(defun org-babel-variable-assignments:plantuml (params)
>> +  "Return a list of PlantUML statements assigning the block's variables."
>
> Could you document what is PARAMS?
I have added more complete docstrings, please let me know if changes are
required.

>> +(defun org-babel-plantuml-make-body (body params)
>> +  "Form PlantUML input string."
>
> Do you mean "Return PlantUML" input string? Also you need to specify
> what are body and params.
Tentatively done.

> Besides, the same applies to `org-babel-plantuml-var-to-plantuml' above.
> Is this function really needed, as it is a mere `format'.
I use this function in the test as well to compare the full text output
so it is convenient to have a separate function.  Alternatively I guess
I could directly test the call to `org-babel-expand-body:generic' but
that seems less interesting as a test (should I remove the test
altogether then?).

The attached patch removes the useless definition of
`org-babel-plantuml-var-to-plantuml' (the regexp is moved to the
`org-babel-variable-assignments:plantuml' function) but keeps the
`org-babel-plantuml-make-body' function, useful for testing.  If you
would like me to remove the `org-babel-plantuml-make-body' function as
well, I will do that (how would you like the test to look like in this
case?)

Thanks for the guidance.

thibault

Attachment: 0001-ob-plantuml.el-Add-support-for-prologue-and-header-v.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]