[PATCH 0/3] org-sbe fixes

[PATCH 0/3] org-sbe fixes

[Vladimir Panteleev]

Inspired by this SE question:

https://emacs.stackexchange.com/questions/32895/how-to-feed-range-from-org-table-filled-with-strings-to-code-block-via-tblfm

Passing table ranges through org-sbe is too hard, and even then the
solution there was not fully correct (did not handle empty cells and
'"' characters properly). As it turns out, we can improve the
situation by addressing some critical shortcomings in the
implementation of org-sbe itself.

There are still quite a few "funny" things left about org-sbe:

 - How var and "var" (as the value for a variable) mean the same
   thing. This matches the syntax used in org-mode documents, but
   doesn't make much sense as part of an API. This creates an
   additional hurdle of using it in tables (references must be further
   $-prefixed, and this $-prefixing will fail in a bad way with
   ranges).

 - The confusing $"string" syntax (or $$3 in tables). It looks like
   some distinct syntax (like bash localizable strings), but $"string"
   is in fact parsed as two tokens ($ and "string").

 - The undocumented syntax where using more than one value for a
   variable implicitly converts it into a list.

 - How these implicit lists are handled differently than single
   values, including when they are also lists.

 - The way it encodes its parameters into a string, just so another
   part of org-mode can decode it later. This happens on two levels,
   org-babel-parse-header-arguments (in org-sbe itself) and parsing of
   the actual argument values.

 - How there are no tests for the entire file, except one that has
   been disabled and left as a TODO since 2011.

This is my first org-mode contribution. I've already done the
copyright assignment process for my first Emacs contribution from last
year, so hopefully that applies here as well.

Vladimir Panteleev (3):
  ob-table: Fix org-sbe's handling of quotes in arguments
  ob-table: Fix org-sbe's handling of list arguments
  ob-table: Mention passing ranges as lists in org-sbe's documentation

 lisp/ob-table.el              | 21 ++++++++++++++-------
 testing/lisp/test-ob-table.el | 30 ++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 7 deletions(-)

-- 
2.16.2

[PATCH 1/3] ob-table: Fix org-sbe's handling of quotes in arguments

[Vladimir Panteleev]

* ob-table.el (org-sbe): (org-sbe func (x $ "a\"b\"c")) did the wrong
thing because org-sbe would simply wrap any $-prefixed value in
quotes, without any escaping. Fix this by using "%S" (instead of
"\"%s\"").

* test-ob-table.el: Add test.
---
 lisp/ob-table.el              |  2 +-
 testing/lisp/test-ob-table.el | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/lisp/ob-table.el b/lisp/ob-table.el
index f6a5c88e4..105aca5e2 100644
--- a/lisp/ob-table.el
+++ b/lisp/ob-table.el
@@ -112,7 +112,7 @@ as shown in the example below.
 				      (prog1 nil (setq quote t))
 				    (prog1
 					(cond
-					 (quote (format "\"%s\"" el))
+					 (quote (format "%S" el))
 					 ((stringp el) (org-no-properties el))
 					 (t el))
 				      (setq quote nil))))
diff --git a/testing/lisp/test-ob-table.el b/testing/lisp/test-ob-table.el
index da136cbee..725cf6bdd 100644
--- a/testing/lisp/test-ob-table.el
+++ b/testing/lisp/test-ob-table.el
@@ -30,6 +30,16 @@
 ;;   (org-test-at-id "6d2ff4ce-4489-4e2a-9c65-e3f71f77d975"
 ;;     (should (= 2 (sbe take-sqrt (n "4"))))))
 
+(ert-deftest test-ob-table/sbe-quote ()
+  "Test that `org-sbe' can correctly handle string arguments containing quotes."
+  (org-test-with-temp-text-in-file "
+#+name: identity
+#+begin_src emacs-lisp :eval yes
+  x
+#+end_src"
+    (should (equal "a\"b\"c"
+		   (eval '(org-sbe identity (x $ "a\"b\"c")))))))
+
 (provide 'test-ob-table)
 
 ;;; test-ob-table.el ends here
-- 
2.16.2

Re: [PATCH 1/3] ob-table: Fix org-sbe's handling of quotes in arguments

[Nicolas Goaziou]

Hello,

Vladimir Panteleev <git@thecybershadow.net> writes:

> * ob-table.el (org-sbe): (org-sbe func (x $ "a\"b\"c")) did the wrong

The dollar character looks strange. What syntax is that?

> -					 (quote (format "\"%s\"" el))
> +					 (quote (format "%S" el))

OK.

> +#+name: identity
> +#+begin_src emacs-lisp :eval yes
> +  x
> +#+end_src"
> +    (should (equal "a\"b\"c"
> +		   (eval '(org-sbe identity (x $ "a\"b\"c")))))))

Why `eval'? Why not simply (org-sbe identity (x ...)) ?

Regards,

-- 
Nicolas Goaziou

Re: [PATCH 1/3] ob-table: Fix org-sbe's handling of quotes in arguments

[Vladimir Panteleev]

Hi,

On 2018-03-12 23:15, Nicolas Goaziou wrote:
> The dollar character looks strange. What syntax is that?

I agree that it is strange. I mentioned it (and its strangeness) in the 
cover-letter. The documentation of org-sbe describes it as well.

>> +#+name: identity
>> +#+begin_src emacs-lisp :eval yes
>> +  x
>> +#+end_src"
>> +    (should (equal "a\"b\"c"
>> +		   (eval '(org-sbe identity (x $ "a\"b\"c")))))))
> 
> Why `eval'? Why not simply (org-sbe identity (x ...)) ?

Ah, I guess that's another "funny" thing about org-sbe that I forgot to 
mention. It is a macro, but it does some things that are rather 
non-macro-y. In this case, eval is required, otherwise it will attempt 
to parse the Org document during Lisp *compilation*, and you will see:

Eager macro-expansion failure: (error "Reference ‘identity’ not found in 
this buffer")
Eager macro-expansion failure: (error "Reference ‘identity’ not found in 
this buffer")
Reference ‘identity’ not found in this buffer

-- 
Best regards,
  Vladimir

Re: [PATCH 1/3] ob-table: Fix org-sbe's handling of quotes in arguments

[Nicolas Goaziou]

Hello,

Vladimir Panteleev <thecybershadow@gmail.com> writes:

> I agree that it is strange. I mentioned it (and its strangeness) in
> the cover-letter. The documentation of org-sbe describes it as well.

AFAICT, it doesn't. "org-sbe" describes a dollar _prefix_, but yours
stands on its own. It prefixes nothing. Also, it seems to apply only to
cell's references, whereas your example provides a strings which is
clearly not a reference. I'm confused.

>>> +#+name: identity
>>> +#+begin_src emacs-lisp :eval yes
>>> +  x
>>> +#+end_src"
>>> +    (should (equal "a\"b\"c"
>>> +		   (eval '(org-sbe identity (x $ "a\"b\"c")))))))
>>
>> Why `eval'? Why not simply (org-sbe identity (x ...)) ?
>
> Ah, I guess that's another "funny" thing about org-sbe that I forgot
> to mention. It is a macro, but it does some things that are rather
> non-macro-y. In this case, eval is required, otherwise it will attempt
> to parse the Org document during Lisp *compilation*, and you will see:
>
> Eager macro-expansion failure: (error "Reference ‘identity’ not found
> in this buffer")
> Eager macro-expansion failure: (error "Reference ‘identity’ not found
> in this buffer")
> Reference ‘identity’ not found in this buffer

What happens if you use (org-sbe "identity" (x ...)) instead?

Regards,

-- 
Nicolas Goaziou                                                0x80A93738

Re: [PATCH 1/3] ob-table: Fix org-sbe's handling of quotes in arguments

[Vladimir Panteleev]

Hi,

On 2018-03-14 13:49, Nicolas Goaziou wrote:
> Vladimir Panteleev <thecybershadow@gmail.com> writes:
>> I agree that it is strange. I mentioned it (and its strangeness) in
>> the cover-letter. The documentation of org-sbe describes it as well.
> 
> AFAICT, it doesn't. "org-sbe" describes a dollar _prefix_, but yours
> stands on its own. It prefixes nothing.

I wrote about this in the cover letter too. $"foo" and $ "foo" are both 
the same thing. In both cases, they are two distinct lisp tokens. The 
way $ is presented as a string/reference "prefix" is through convention 
only.

Or are you objecting on stylistic grounds, that the test case from my 
patch doesn't follow the convention of omitting whitespace after the $ 
token, thus making it look like a prefix? This is, of course, 
subjective, but I would prefer to not perpetuate the illusion that 
$"foo" is some magical Emacs Lisp language syntax for a new kind of 
string literal, at least in the test suite.

> Also, it seems to apply only to
> cell's references, whereas your example provides a strings which is
> clearly not a reference. I'm confused.

The references are substituted with string literals before the $-prefix 
handling occurs. This is why it doesn't work with ranges.

I agree that it is all very confusing, and there is a lot of room for 
improvement. That doesn't stand in the way of this patch series, though.

> What happens if you use (org-sbe "identity" (x ...)) instead?

No difference. As can be seen from org-sbe's implementation, the 
normalization of symbols and string literals occurs before attempting to 
resolve references.

-- 
Best regards,
  Vladimir

Re: [PATCH 1/3] ob-table: Fix org-sbe's handling of quotes in arguments

[Nicolas Goaziou]

Vladimir Panteleev <thecybershadow@gmail.com> writes:

> I wrote about this in the cover letter too. $"foo" and $ "foo" are
> both the same thing.

Just to make it clear: I read the cover letter. My confusion doesn't
come from the fact I may not have read it.

> In both cases, they are two distinct lisp tokens.
> The way $ is presented as a string/reference "prefix" is through
> convention only.
>
> Or are you objecting on stylistic grounds, that the test case from my
> patch doesn't follow the convention of omitting whitespace after the
> $ token, thus making it look like a prefix? This is, of course,
> subjective, but I would prefer to not perpetuate the illusion that
> $"foo" is some magical Emacs Lisp language syntax for a new kind of
> string literal, at least in the test suite.

I disagree. You are testing an implementation detail here: the fact that
"$" is not necessarily a prefix. According to the docstring, it should
be, so the test should use that, too. What if we rewrite `org-sbe' at
some point?

The same goes for the next string. $"foo", or in your case, $"a\"b\"c"
means nothing in `org-sbe' context. A reference should follow the dollar
character, per `org-sbe' docstring. I suggest to make an equivalent test
with, e.g., $"@1$1", where @1$1 refers to a field containing "a\"b\"c"
or some such.

I suggest to stick to the specifications, which, in this case, are the
docstring, not the code.

> The references are substituted with string literals before the
> $-prefix handling occurs. This is why it doesn't work with ranges.

I understand it doesn't work with ranges, but, per above, your tests are
confusing, IMO.

> I agree that it is all very confusing, and there is a lot of room for
> improvement. That doesn't stand in the way of this patch series,
> though.

I don't think anything stands in the way, but I'd rather get the tests
right, first.

> No difference. As can be seen from org-sbe's implementation, the
> normalization of symbols and string literals occurs before attempting
> to resolve references.

Something is rotten in the state of "ob-table.el", may think.

Re: [PATCH 1/3] ob-table: Fix org-sbe's handling of quotes in arguments

[Vladimir Panteleev]

On 2018-03-14 15:00, Nicolas Goaziou wrote:
> Vladimir Panteleev <thecybershadow@gmail.com> writes:
>> I wrote about this in the cover letter too. $"foo" and $ "foo" are
>> both the same thing.
> 
> Just to make it clear: I read the cover letter. My confusion doesn't
> come from the fact I may not have read it.

In that case, I need to apologize for not making it sufficiently clear, 
as we're reiterating the same points without making much progress.

> I disagree. You are testing an implementation detail here: the fact that
> "$" is not necessarily a prefix. According to the docstring, it should
> be, so the test should use that, too. What if we rewrite `org-sbe' at
> some point?

I'm sorry if I didn't explain it properly in my previous messages. I'll 
try again.

The fact that $"foo" and $ "foo" mean the same thing is not an 
implementation detail of org-sbe. It is a consequence of Emacs Lisp grammar.

Try evaluating '($"foo") and '($ "foo") in M-:. You will get the same 
result, because after tokenization, their representation is identical. 
The whitespace between $ and "foo" is completely insignificant.

In fact, if you try to evaluate $"foo" by itself, you will get "Trailing 
garbage following expression", because, as I mentioned before, $ and 
"foo" are two distinct tokens.

There is absolutely nothing that anyone could change in org-sbe that 
would make $ "foo" mean something other than $"foo".

So, whether the test case has a space between $ and "a\"b\"c" is as 
relevant as whether it has comments, or uses tabs instead of spaces for 
indentation.

I hope this explanation can put this issue to rest.

> The same goes for the next string. $"foo", or in your case, $"a\"b\"c"
> means nothing in `org-sbe' context. A reference should follow the dollar
> character, per `org-sbe' docstring. I suggest to make an equivalent test
> with, e.g., $"@1$1", where @1$1 refers to a field containing "a\"b\"c"
> or some such.

As I've mentioned, table references are resolved before org-sbe is 
invoked. By the time org-sbe begins executing, all table references will 
have already been replaced by string literals. Using a table reference 
confounds the matter because it is testing more than just org-sbe.

org-sbe's docstring is misleading: $ is not a way to quote just table 
references, but any string literals in general. Had it been otherwise 
(i.e. $-prefixing being part of the table reference syntax), there would 
be no way to pass a string literal (which isn't an interpolated table 
cell value) to the indicated function. The doc string should probably be 
improved in this regard.

Adding a test which uses a table reference instead of a string literal 
won't hurt, but it would be testing several layers at once, and, 
assuming cell value interpolation into emacs lisp table formulas is 
already tested somewhere else, superfluous.

-- 
Best regards,
  Vladimir

Re: [PATCH 1/3] ob-table: Fix org-sbe's handling of quotes in arguments

[Nicolas Goaziou]

Hello,

Vladimir Panteleev <thecybershadow@gmail.com> writes:

> On 2018-03-14 15:00, Nicolas Goaziou wrote:

>> I disagree. You are testing an implementation detail here: the fact that
>> "$" is not necessarily a prefix. According to the docstring, it should
>> be, so the test should use that, too. What if we rewrite `org-sbe' at
>> some point?
>
> I'm sorry if I didn't explain it properly in my previous messages.
> I'll try again.
>
> The fact that $"foo" and $ "foo" mean the same thing is not an
> implementation detail of org-sbe. It is a consequence of Emacs Lisp
> grammar.

[...]

> So, whether the test case has a space between $ and "a\"b\"c" is as
> relevant as whether it has comments, or uses tabs instead of spaces
> for indentation.
>
> I hope this explanation can put this issue to rest.

We're clearly mis-communicating. I know the difference between a symbol
and a string, and how `read' operates. I think what puzzles me is some
design choices made in `org-sbe', and the fact that the second note of
its docstring is clear as mud.

> org-sbe's docstring is misleading:

I agree.

> $ is not a way to quote just table references, but any string literals
> in general. Had it been otherwise (i.e. $-prefixing being part of the
> table reference syntax), there would be no way to pass a string
> literal (which isn't an interpolated table cell value) to the
> indicated function. The doc string should probably be improved in this
> regard.

I consider it to be a bug if you need to write $"string" instead of
"string" in any `org-sbe' call. We should not test such a mis-feature,
which should be removed.

> Adding a test which uses a table reference instead of a string literal
> won't hurt, but it would be testing several layers at once, and,
> assuming cell value interpolation into emacs lisp table formulas is
> already tested somewhere else, superfluous.

Let me insist on this. $"string" (or $ "string") is very wrong. Please
do not write your tests on top of this. IIUC, `org-sbe' is about passing
table fields into source blocks. Let's test that instead, with a table,
and a source block. I don't mind superfluous coverage.

It is just a matter of tests. Your changes sound good. So, would you
mind adjusting your tests so we can move forward?

Regards,

-- 
Nicolas Goaziou

Re: [PATCH 1/3] ob-table: Fix org-sbe's handling of quotes in arguments

[Vladimir Panteleev]

Hi,

On 2018-03-18 22:24, Nicolas Goaziou wrote:
> We're clearly mis-communicating. I know the difference between a symbol
> and a string, and how `read' operates. I think what puzzles me is some
> design choices made in `org-sbe', and the fact that the second note of
> its docstring is clear as mud.

Ah! Well, thanks for the clarification, and apologies for the confusion.

> I consider it to be a bug if you need to write $"string" instead of
> "string" in any `org-sbe' call. We should not test such a mis-feature,
> which should be removed.

Agreed completely, but, wouldn't this be a breaking change? So far I've 
tried to avoid breaking any meaningful existing uses of org-sbe.

> Let me insist on this. $"string" (or $ "string") is very wrong. Please
> do not write your tests on top of this. IIUC, `org-sbe' is about passing
> table fields into source blocks. Let's test that instead, with a table,
> and a source block. I don't mind superfluous coverage.
> 
> It is just a matter of tests. Your changes sound good. So, would you
> mind adjusting your tests so we can move forward?

No problem. To clarify, would you prefer tests which still use 
$-prefixing, but with a table cell reference to get the value in the 
cell (i.e. $$2), or tests which do not use $-prefixing in order to 
interpret the string in the table cell as a reference to another block 
in the org-mode document?

-- 
Best regards,
  Vladimir

Re: [PATCH 1/3] ob-table: Fix org-sbe's handling of quotes in arguments

[Nicolas Goaziou]

Vladimir Panteleev <thecybershadow@gmail.com> writes:

> Ah! Well, thanks for the clarification, and apologies for the
> confusion.

Well, thanks for bearing with me.

>> I consider it to be a bug if you need to write $"string" instead of
>> "string" in any `org-sbe' call. We should not test such a mis-feature,
>> which should be removed.
>
> Agreed completely, but, wouldn't this be a breaking change? So far
> I've tried to avoid breaking any meaningful existing uses of org-sbe.

We probably need to break `org-sbe' so as to improve it. In particular,
its syntax ought to evolve, as you pointed out. However, such changes
would obviously go into master branch, not in maint.

> No problem. To clarify, would you prefer tests which still use
> $-prefixing, but with a table cell reference to get the value in the
> cell (i.e. $$2), or tests which do not use $-prefixing in order to
> interpret the string in the table cell as a reference to another block
> in the org-mode document?

At the moment, both are features described in `org-sbe' docstring, so
I think they should be both tested, if appropriate. However, I had the
former in mind when I suggested to change tests; the latter sounds very
much like over-engineering.

[PATCH v2 0/3] org-sbe fixes

[Vladimir Panteleev]

OK, here is v2 with table-based tests.

Vladimir Panteleev (3):
  ob-table: Fix org-sbe's handling of quotes in cell values
  ob-table: Fix org-sbe's handling of list arguments
  ob-table: Mention passing ranges as lists in org-sbe's documentation

 lisp/ob-table.el              | 21 +++++++++++++-------
 testing/lisp/test-ob-table.el | 45 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 7 deletions(-)

-- 
2.16.2

[PATCH v2 1/3] ob-table: Fix org-sbe's handling of quotes in cell values

[Vladimir Panteleev]

* ob-table.el (org-sbe): org-sbe did the wrong thing when given a
reference to a cell containing quotes or backslashes, because it would
simply wrap any $-prefixed value in quotes, without any escaping. Fix
this by using "%S" (instead of "\"%s\"").

* test-ob-table.el: Add test.
---
 lisp/ob-table.el              |  2 +-
 testing/lisp/test-ob-table.el | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/lisp/ob-table.el b/lisp/ob-table.el
index f6a5c88e4..105aca5e2 100644
--- a/lisp/ob-table.el
+++ b/lisp/ob-table.el
@@ -112,7 +112,7 @@ as shown in the example below.
 				      (prog1 nil (setq quote t))
 				    (prog1
 					(cond
-					 (quote (format "\"%s\"" el))
+					 (quote (format "%S" el))
 					 ((stringp el) (org-no-properties el))
 					 (t el))
 				      (setq quote nil))))
diff --git a/testing/lisp/test-ob-table.el b/testing/lisp/test-ob-table.el
index da136cbee..3d9b1d160 100644
--- a/testing/lisp/test-ob-table.el
+++ b/testing/lisp/test-ob-table.el
@@ -30,6 +30,28 @@
 ;;   (org-test-at-id "6d2ff4ce-4489-4e2a-9c65-e3f71f77d975"
 ;;     (should (= 2 (sbe take-sqrt (n "4"))))))
 
+(ert-deftest test-ob-table/sbe-quote ()
+  "Test that `org-sbe' can correctly handle cell values containing quotes."
+  (org-test-table-target-expect
+   "
+#+name: identity
+#+begin_src emacs-lisp :eval yes
+  x
+#+end_src
+
+| a\"b\"c | replace |
+"
+   "
+#+name: identity
+#+begin_src emacs-lisp :eval yes
+  x
+#+end_src
+
+| a\"b\"c | a\"b\"c |
+"
+   1
+   "#+TBLFM: $2 = '(org-sbe identity (x $$1))"))
+
 (provide 'test-ob-table)
 
 ;;; test-ob-table.el ends here
-- 
2.16.2

[PATCH v2 2/3] ob-table: Fix org-sbe's handling of list arguments

[Vladimir Panteleev]

* ob-table.el (org-sbe): Add an explicit case for handling list
arguments. This avoids doing the wrong thing (%s-formatting a list,
thus losing syntax like double-quotes). This enables passing org-table
ranges through org-sbe in a simple and correct manner.

* test-ob-table.el: Add test.
---
 lisp/ob-table.el              | 17 +++++++++++------
 testing/lisp/test-ob-table.el | 23 +++++++++++++++++++++++
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/lisp/ob-table.el b/lisp/ob-table.el
index 105aca5e2..17810dd74 100644
--- a/lisp/ob-table.el
+++ b/lisp/ob-table.el
@@ -132,12 +132,17 @@ as shown in the example below.
                                  "("
                                  (mapconcat
                                   (lambda (var-spec)
-                                    (if (> (length (cdr var-spec)) 1)
-                                        (format "%S='%S"
-                                                (car var-spec)
-                                                (mapcar #'read (cdr var-spec)))
-                                      (format "%S=%s"
-                                              (car var-spec) (cadr var-spec))))
+                                    (cond
+				     ((> (length (cdr var-spec)) 1)
+				      (format "%S='%S"
+					      (car var-spec)
+					      (mapcar #'read (cdr var-spec))))
+				     ((stringp (cadr var-spec))
+				      (format "%S=%s"
+                                              (car var-spec) (cadr var-spec)))
+				     (t
+				      (format "%S=%S"
+                                              (car var-spec) (cadr var-spec)))))
                                   ',variables ", ")
                                  ")")))))
                    (org-babel-execute-src-block
diff --git a/testing/lisp/test-ob-table.el b/testing/lisp/test-ob-table.el
index 3d9b1d160..fb6d05796 100644
--- a/testing/lisp/test-ob-table.el
+++ b/testing/lisp/test-ob-table.el
@@ -52,6 +52,29 @@
    1
    "#+TBLFM: $2 = '(org-sbe identity (x $$1))"))
 
+(ert-deftest test-ob-table/sbe-list ()
+  "Test that `org-sbe' can correctly handle ranges as lists."
+  (org-test-table-target-expect
+   "
+#+name: concat
+#+begin_src emacs-lisp :eval yes
+  (mapconcat #'identity x \"\")
+#+end_src
+
+| foo | bar | replace |
+"
+   "
+#+name: concat
+#+begin_src emacs-lisp :eval yes
+  (mapconcat #'identity x \"\")
+#+end_src
+
+| foo | bar | foobar |
+"
+   1
+   "#+TBLFM: $3 = '(org-sbe concat (x   (list $1..$2)))"
+   "#+TBLFM: $3 = '(org-sbe concat (x $ (list $1..$2)))"))
+
 (provide 'test-ob-table)
 
 ;;; test-ob-table.el ends here
-- 
2.16.2

[PATCH v2 3/3] ob-table: Mention passing ranges as lists in org-sbe's documentation

[Vladimir Panteleev]

* ob-table.el (org-sbe): Add documentation note.
---
 lisp/ob-table.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/ob-table.el b/lisp/ob-table.el
index 17810dd74..d11df9abc 100644
--- a/lisp/ob-table.el
+++ b/lisp/ob-table.el
@@ -89,6 +89,8 @@ NOTE: By default, string variable names are interpreted as
 references to source-code blocks, to force interpretation of a
 cell's value as a string, prefix the identifier a \"$\" (e.g.,
 \"$$2\" instead of \"$2\" or \"$@2$2\" instead of \"@2$2\").
+This will not work with a range; instead, pass it as a list,
+e.g. (org-sbe fun (r (list $1..$2))).
 
 NOTE: It is also possible to pass header arguments to the code
 block.  In this case a table cell should hold the string value of
-- 
2.16.2

Re: [PATCH v2 0/3] org-sbe fixes

[Nicolas Goaziou]

Hello,

Vladimir Panteleev <git@thecybershadow.net> writes:

> OK, here is v2 with table-based tests.
>
> Vladimir Panteleev (3):
>   ob-table: Fix org-sbe's handling of quotes in cell values
>   ob-table: Fix org-sbe's handling of list arguments
>   ob-table: Mention passing ranges as lists in org-sbe's documentation

Applied. Thank you!

Regards,

-- 
Nicolas Goaziou

Re: [PATCH v2 0/3] org-sbe fixes

[Alan Schmitt]

[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]

Hello,

On 2018-03-20 00:07, Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Hello,
>
> Vladimir Panteleev <git@thecybershadow.net> writes:
>
>> OK, here is v2 with table-based tests.
>>
>> Vladimir Panteleev (3):
>>   ob-table: Fix org-sbe's handling of quotes in cell values
>>   ob-table: Fix org-sbe's handling of list arguments
>>   ob-table: Mention passing ranges as lists in org-sbe's documentation
>
> Applied. Thank you!

I think this has been applied to the maint branch, but I cannot find a
mention of it in the ORG-NEWS file. Maybe it should be mentioned there
as it is a breaking change. For instance,(I had to modify table formulas like

#+TBLFM: @2$3..@>>$3='(org-sbe call_ledger (bucket (concat "\"" $1 "\"")) (prefix "\"Bucket:Expenses:\""))

into

#+TBLFM: @2$3..@>>$3='(org-sbe call_ledger (bucket $$1) (prefix "\"Bucket:Expenses:\""))

Best,

Alan

-- 
OpenPGP Key ID : 040D0A3B4ED2E5C7
Monthly Athmospheric CO₂, Mauna Loa Obs. 2018-02: 408.35, 2017-02: 406.42

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 528 bytes --]

Re: [PATCH v2 0/3] org-sbe fixes

[Nicolas Goaziou]

Hello,

Alan Schmitt <alan.schmitt@polytechnique.org> writes:

> I think this has been applied to the maint branch, but I cannot find a
> mention of it in the ORG-NEWS file. Maybe it should be mentioned there
> as it is a breaking change. For instance,(I had to modify table formulas like
>
> #+TBLFM: @2$3..@>>$3='(org-sbe call_ledger (bucket (concat "\"" $1 "\"")) (prefix "\"Bucket:Expenses:\""))
>
> into
>
> #+TBLFM: @2$3..@>>$3='(org-sbe call_ledger (bucket $$1) (prefix
> "\"Bucket:Expenses:\""))

I'm going to revert these changes from maint, then. I though that was
innocuous.

Vladimir, would you mind writing a small entry in ORG-NEWS about the
induced changes in master?

Thank you.

Regards,

-- 
Nicolas Goaziou

Re: [PATCH v2 0/3] org-sbe fixes

[Vladimir Panteleev]

On 2018-03-25 20:26, Nicolas Goaziou wrote:
>> I think this has been applied to the maint branch, but I cannot find a
>> mention of it in the ORG-NEWS file. Maybe it should be mentioned there
>> as it is a breaking change. For instance,(I had to modify table formulas like

Sorry, I did not intend this to be a breaking change (for any reasonably 
valid use cases).

The problem is that because org-sbe is a macro, it attempts to parse the 
sexpr without evaluating it. The intention was to check what kind of 
lisp object the argument is, and preserve the old behavior if it's a 
string, but do something more sensible if it's a list. I.e. "foo" and 
(identity "foo") are currently treated differently in org-sbe arguments. 
However, it currently can't do that because the actual evaluation occurs 
elsewhere (the sexpr is stringified mostly-as-is into a babel header and 
then parsed as such).

One ugly fix would be to patch over this by checking for some common 
lisp forms that result in a list, such as (list ...) and (quote ...), 
and treat the remaining ones in a backwards-compatible way. However, the 
more time I spend on org-sbe, the more I think that perhaps a better 
approach would be to deprecate org-sbe (or outright remove it, I could 
argue that due to the overwhelming number of flaws its mere presence is 
downright harmful), and replace it with something more sensible: a 
function (not macro) defined somewhere other than ob-table (because it 
is not specific to tables) without any of the crazy string escaping logic.

What do you think?

-- 
Best regards,
  Vladimir

Re: [PATCH v2 0/3] org-sbe fixes

[Nicolas Goaziou]

Hello,

Vladimir Panteleev <thecybershadow@gmail.com> writes:

> One ugly fix would be to patch over this by checking for some common
> lisp forms that result in a list, such as (list ...) and (quote ...),
> and treat the remaining ones in a backwards-compatible way. However,
> the more time I spend on org-sbe, the more I think that perhaps
> a better approach would be to deprecate org-sbe (or outright remove
> it, I could argue that due to the overwhelming number of flaws its
> mere presence is downright harmful), and replace it with something
> more sensible: a function (not macro) defined somewhere other than
> ob-table (because it is not specific to tables) without any of the
> crazy string escaping logic.
>
> What do you think?

I have reverted your changes from maint.

I agree "org-sbe" should be rewritten. However, it is specific to tables
because it is meant to be used in a TBLFM line. Its replacement could
live in "org-table.el", though.

Regards,

-- 
Nicolas Goaziou                                                0x80A93738

Re: [PATCH v2 0/3] org-sbe fixes

[Vladimir Panteleev]

Hi,

On 2018-03-25 21:06, Nicolas Goaziou wrote:
> I agree "org-sbe" should be rewritten. However, it is specific to tables
> because it is meant to be used in a TBLFM line. Its replacement could
> live in "org-table.el", though.

What about org-sbe is specific to tables?

I'm thinking that the generic replacement could be used elsewhere, e.g.:

(with-current-buffer some-org-mode-buffer
   (org-sbe-v2 "func" ...args...))

The main obstacle preventing using the current org-sbe in this manner is 
that it is a macro, and will eagerly evaluate some things that it shouldn't.

-- 
Best regards,
  Vladimir

Re: [PATCH v2 0/3] org-sbe fixes

[Nicolas Goaziou]

Hello,

Vladimir Panteleev <thecybershadow@gmail.com> writes:

> Hi,
>
> On 2018-03-25 21:06, Nicolas Goaziou wrote:
>> I agree "org-sbe" should be rewritten. However, it is specific to tables
>> because it is meant to be used in a TBLFM line. Its replacement could
>> live in "org-table.el", though.
>
> What about org-sbe is specific to tables?
>
> I'm thinking that the generic replacement could be used elsewhere, e.g.:
>
> (with-current-buffer some-org-mode-buffer
>   (org-sbe-v2 "func" ...args...))

The generic replacement of table cells, right. Otherwise you have inline
Babel calls.

Regards,

-- 
Nicolas Goaziou                                                0x80A93738

Re: [PATCH v2 0/3] org-sbe fixes

[Vladimir Panteleev]

On 2018-03-26 20:16, Nicolas Goaziou wrote:
> Hello,
> 
> Vladimir Panteleev <thecybershadow@gmail.com> writes:
>> What about org-sbe is specific to tables?
>>
>> I'm thinking that the generic replacement could be used elsewhere, e.g.:
>>
>> (with-current-buffer some-org-mode-buffer
>>    (org-sbe-v2 "func" ...args...))
> 
> The generic replacement of table cells, right. Otherwise you have inline
> Babel calls.

Sorry, I'm not following. By "generic replacement" I meant a replacement 
(new function that will replace the current org-sbe) that is generic 
(not restricted to being used in table formulas by design).

I think you seem to have interpreted to mean substitution of table cell 
references. This is actually not pertaining org-sbe or its replacement, 
because it is not done in org-sbe, but by code invoking it.

By inline Babel calls, do you mean as described in 
https://orgmode.org/manual/Evaluating-code-blocks.html ? I see there 
"org-babel-execute-src-block", but it seems to attempt to infer the src 
block to execute rather than letting the caller specify one.

So, as far as I can see, org-sbe's successor doesn't need to have 
anything specific to tables, or have I missed something?

-- 
Best regards,
  Vladimir

Re: [PATCH v2 0/3] org-sbe fixes

[Nicolas Goaziou]

Vladimir Panteleev <thecybershadow@gmail.com> writes:

> By inline Babel calls, do you mean as described in
> https://orgmode.org/manual/Evaluating-code-blocks.html ? I see there
> "org-babel-execute-src-block", but it seems to attempt to infer the
> src block to execute rather than letting the caller specify one.

I'm talking about call_foo() syntax. I see no fundamental difference
between (org-sbe-v2 "func" ...) and call_func(...).

Re: [PATCH v2 0/3] org-sbe fixes

[Vladimir Panteleev]

On 2018-03-26 21:42, Nicolas Goaziou wrote:
> I'm talking about call_foo() syntax. I see no fundamental difference
> between (org-sbe-v2 "func" ...) and call_func(...).

Sorry, how do you invoke call_func from lisp, or a table formula?

-- 
Best regards,
  Vladimir

Re: [PATCH v2 0/3] org-sbe fixes

[Nicolas Goaziou]

Hello,

Vladimir Panteleev <thecybershadow@gmail.com> writes:

> On 2018-03-26 21:42, Nicolas Goaziou wrote:
>> I'm talking about call_foo() syntax. I see no fundamental difference
>> between (org-sbe-v2 "func" ...) and call_func(...).
>
> Sorry, how do you invoke call_func from lisp, or a table formula?

I'm not sure there is a real use-case for the first case. Remember you
can chain source blocks.

The second is the reason why I'm suggesting to move org-sbe-vs to
"org-table.el".

Regards,

-- 
Nicolas Goaziou

Re: [PATCH v2 0/3] org-sbe fixes

[Alan Schmitt]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]

On 2018-03-25 23:06, Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> I have reverted your changes from maint.

Should there be a new release? This change is present in 9.1.8.

Best,

Alan

-- 
OpenPGP Key ID : 040D0A3B4ED2E5C7
Monthly Athmospheric CO₂, Mauna Loa Obs. 2018-02: 408.35, 2017-02: 406.42

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 528 bytes --]

Re: [PATCH v2 0/3] org-sbe fixes

[Nicolas Goaziou]

Hello,

Alan Schmitt <alan.schmitt@polytechnique.org> writes:

> On 2018-03-25 23:06, Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:
>
>> I have reverted your changes from maint.
>
> Should there be a new release? This change is present in 9.1.8.

I /think/ I released Org 9.1.9, but I usually goof it up, so I'm Cc'ing
Bastien.

Regards,

-- 
Nicolas Goaziou                                                0x80A93738

Re: [PATCH v2 0/3] org-sbe fixes

[Bastien]

Hello,

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Alan Schmitt <alan.schmitt@polytechnique.org> writes:
>
>> On 2018-03-25 23:06, Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:
>>
>>> I have reverted your changes from maint.
>>
>> Should there be a new release? This change is present in 9.1.8.
>
> I /think/ I released Org 9.1.9, but I usually goof it up, so I'm Cc'ing
> Bastien.

Yes, something went wrong in the release process, I just released
9.1.9 manually.

All best,

-- 
 Bastien

[PATCH 2/3] ob-table: Fix org-sbe's handling of list arguments

[Vladimir Panteleev]

* ob-table.el (org-sbe): Add an explicit case for handling list
arguments. This avoids doing the wrong thing (%s-formatting a list,
thus losing syntax like double-quotes). This enables passing org-table
ranges through org-sbe in a simple and correct manner.

* test-ob-table.el: Add test.
---
 lisp/ob-table.el              | 17 +++++++++++------
 testing/lisp/test-ob-table.el | 20 ++++++++++++++++++++
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/lisp/ob-table.el b/lisp/ob-table.el
index 105aca5e2..17810dd74 100644
--- a/lisp/ob-table.el
+++ b/lisp/ob-table.el
@@ -132,12 +132,17 @@ as shown in the example below.
                                  "("
                                  (mapconcat
                                   (lambda (var-spec)
-                                    (if (> (length (cdr var-spec)) 1)
-                                        (format "%S='%S"
-                                                (car var-spec)
-                                                (mapcar #'read (cdr var-spec)))
-                                      (format "%S=%s"
-                                              (car var-spec) (cadr var-spec))))
+                                    (cond
+				     ((> (length (cdr var-spec)) 1)
+				      (format "%S='%S"
+					      (car var-spec)
+					      (mapcar #'read (cdr var-spec))))
+				     ((stringp (cadr var-spec))
+				      (format "%S=%s"
+                                              (car var-spec) (cadr var-spec)))
+				     (t
+				      (format "%S=%S"
+                                              (car var-spec) (cadr var-spec)))))
                                   ',variables ", ")
                                  ")")))))
                    (org-babel-execute-src-block
diff --git a/testing/lisp/test-ob-table.el b/testing/lisp/test-ob-table.el
index 725cf6bdd..40cc877d8 100644
--- a/testing/lisp/test-ob-table.el
+++ b/testing/lisp/test-ob-table.el
@@ -40,6 +40,26 @@
     (should (equal "a\"b\"c"
 		   (eval '(org-sbe identity (x $ "a\"b\"c")))))))
 
+(ert-deftest test-ob-table/sbe-list ()
+  "Test that `org-sbe' can correctly handle list arguments."
+  (org-test-with-temp-text-in-file "
+#+name: concat
+#+begin_src emacs-lisp :eval yes
+  (mapconcat #'identity x \"\")
+#+end_src"
+    (should (equal "foobar"
+		   (eval '(org-sbe concat (x '("foo" "bar"))))))))
+
+(ert-deftest test-ob-table/sbe-$-list ()
+  "Test that `org-sbe' can correctly handle $-prefixed list arguments."
+  (org-test-with-temp-text-in-file "
+#+name: concat
+#+begin_src emacs-lisp :eval yes
+  (mapconcat #'identity x \"\")
+#+end_src"
+    (should (equal "foobar"
+		   (eval '(org-sbe concat (x $ '("foo" "bar"))))))))
+
 (provide 'test-ob-table)
 
 ;;; test-ob-table.el ends here
-- 
2.16.2

[PATCH 3/3] ob-table: Mention passing ranges as lists in org-sbe's documentation

[Vladimir Panteleev]

* ob-table.el (org-sbe): Add documentation note.
---
 lisp/ob-table.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/ob-table.el b/lisp/ob-table.el
index 17810dd74..d11df9abc 100644
--- a/lisp/ob-table.el
+++ b/lisp/ob-table.el
@@ -89,6 +89,8 @@ NOTE: By default, string variable names are interpreted as
 references to source-code blocks, to force interpretation of a
 cell's value as a string, prefix the identifier a \"$\" (e.g.,
 \"$$2\" instead of \"$2\" or \"$@2$2\" instead of \"@2$2\").
+This will not work with a range; instead, pass it as a list,
+e.g. (org-sbe fun (r (list $1..$2))).
 
 NOTE: It is also possible to pass header arguments to the code
 block.  In this case a table cell should hold the string value of
-- 
2.16.2