[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
0001-ob-plantuml.el-Add-support-for-prologue-and-header-v.patch
Description: Text Data
- [O] Add preamble support to ob-plantuml.el, Thibault Marin, 2016/12/02
- Re: [O] Add preamble support to ob-plantuml.el, Nicolas Goaziou, 2016/12/05
- Re: [O] Add preamble support to ob-plantuml.el, Thibault Marin, 2016/12/05
- Re: [O] Add preamble support to ob-plantuml.el, Nicolas Goaziou, 2016/12/06
- Re: [O] Add preamble support to ob-plantuml.el, Rainer M Krug, 2016/12/06
- Re: [O] Add preamble support to ob-plantuml.el, Thibault Marin, 2016/12/06
- Re: [O] Add preamble support to ob-plantuml.el, Nicolas Goaziou, 2016/12/09
- Re: [O] Add preamble support to ob-plantuml.el,
Thibault Marin <=
- Re: [O] Add preamble support to ob-plantuml.el, Nicolas Goaziou, 2016/12/10
- Re: [O] Add preamble support to ob-plantuml.el, Thibault Marin, 2016/12/10
- Re: [O] Add preamble support to ob-plantuml.el, Nicolas Goaziou, 2016/12/10