fa5fd6351605912ec75e783cb62649 breaks org-babel-script-escape for ob-ruby

fa5fd6351605912ec75e783cb62649 breaks org-babel-script-escape for ob-ruby

[Matthew MacLean]

[-- Attachment #1.1: Type: text/plain, Size: 905 bytes --]

Yo~!

fa5fd6351605912ec75e783cb626497b1ebe471e introduced a change where
org-babel-script-escape stopped accepting numbers. This caused an issue in
ob-ruby.el where when trying to evaluate something like "2 + 2", you would
get the message:

  `org-babel-script-escape' expects a string

This broke evaluation of Ruby code blocks.

I suspect this is not the only location where this problem might arise, so
I am submitting a patch so the function simply returns numbers if they are
passed in rather than dying. (Because numbers don't need to be escaped, and
this was the previous behaviour.)

Thanks..!

By the way, sorry if this ends up being a duplicate... I learned not so
long ago I needed to subscribe to actually post here and wasn't sure if my
last post went to /dev/null or is just awaiting moderation. (I tested to
make sure the patch still works with the most recent commit of the maint
branch.)

[-- Attachment #1.2: Type: text/html, Size: 1048 bytes --]

[-- Attachment #2: org-babel-script-escape-numbers.patch --]
[-- Type: text/x-patch, Size: 2814 bytes --]

diff --git a/lisp/ob-core.el b/lisp/ob-core.el
index e3abe97..01c4da8 100644
--- a/lisp/ob-core.el
+++ b/lisp/ob-core.el
@@ -2813,34 +2813,37 @@ block but are passed literally to the \"example-block\"."
       (error "Unterminated string in `org-babel-script-escape'"))
     (apply #'string (reverse out))))
 
-(defun org-babel-script-escape (str &optional force)
-  "Safely convert tables into elisp lists."
-  (unless (stringp str)
-    (error "`org-babel-script-escape' expects a string"))
-  (let ((escaped
-	 (cond
-	  ((and (> (length str) 2)
-		(or (and (string-equal "[" (substring str 0 1))
-			 (string-equal "]" (substring str -1)))
-		    (and (string-equal "{" (substring str 0 1))
-			 (string-equal "}" (substring str -1)))
-		    (and (string-equal "(" (substring str 0 1))
-			 (string-equal ")" (substring str -1)))))
-
-	   (concat "'" (org-babel--script-escape-inner str)))
-	  ((or force
-	       (and (> (length str) 2)
-		    (or (and (string-equal "'" (substring str 0 1))
-			     (string-equal "'" (substring str -1)))
-			;; We need to pass double-quoted strings
-			;; through the backslash-twiddling bits, even
-			;; though we don't need to change their
-			;; delimiters.
-			(and (string-equal "\"" (substring str 0 1))
-			     (string-equal "\"" (substring str -1))))))
-	   (org-babel--script-escape-inner str))
-	  (t str))))
-    (condition-case nil (org-babel-read escaped) (error escaped))))
+(defun org-babel-script-escape (val &optional force)
+  "Safely convert passed in values (including collections of
+them; tables) into elisp lists."
+  (if (numberp val)
+      val
+    (unless (stringp val)
+      (error "`org-babel-script-escape' expects a string or number"))
+    (let ((escaped
+	   (cond
+	    ((and (> (length val) 2)
+		  (or (and (string-equal "[" (substring val 0 1))
+			   (string-equal "]" (substring val -1)))
+		      (and (string-equal "{" (substring val 0 1))
+			   (string-equal "}" (substring val -1)))
+		      (and (string-equal "(" (substring val 0 1))
+			   (string-equal ")" (substring val -1)))))
+
+	     (concat "'" (org-babel--script-escape-inner val)))
+	    ((or force
+		 (and (> (length val) 2)
+		      (or (and (string-equal "'" (substring val 0 1))
+			       (string-equal "'" (substring val -1)))
+			  ;; We need to pass double-quoted strings
+			  ;; through the backslash-twiddling bits, even
+			  ;; though we don't need to change their
+			  ;; delimiters.
+			  (and (string-equal "\"" (substring val 0 1))
+			       (string-equal "\"" (substring val -1))))))
+	     (org-babel--script-escape-inner val))
+	    (t val))))
+      (condition-case nil (org-babel-read escaped) (error escaped)))))
 
 (defun org-babel-read (cell &optional inhibit-lisp-eval)
   "Convert the string value of CELL to a number if appropriate.

Re: fa5fd6351605912ec75e783cb62649 breaks org-babel-script-escape for ob-ruby

[Kyle Meyer]

Matthew MacLean <archenoth@gmail.com> wrote:
> Yo~!
>
> fa5fd6351605912ec75e783cb626497b1ebe471e introduced a change where
> org-babel-script-escape stopped accepting numbers. This caused an issue in
> ob-ruby.el where when trying to evaluate something like "2 + 2", you would
> get the message:
>
>   `org-babel-script-escape' expects a string
>
> This broke evaluation of Ruby code blocks.
>
> I suspect this is not the only location where this problem might
> arise,

Perhaps ob-ruby.el is to blame.

#+begin_src ruby
  2 + 2
#+end_src

The above snippet calls org-babel-script-escape twice: first with "4"
and then with 4.  The first one happens with the org-babel-ruby-evaluate
call (which leads to an org-babel-ruby-table-or-string call) in
org-babel-execute:ruby and the second happens with the direct call to
org-babel-ruby-table-or-string in org-babel-execute:ruby.

Is the results-params check and call to org-babel-ruby-table-or-string
in org-babel-ruby-evaluate needed?  It seems like org-babel-execute
already covers this processing.

--
Kyle

Re: fa5fd6351605912ec75e783cb62649 breaks org-babel-script-escape for ob-ruby

[Kyle Meyer]

Kyle Meyer <kyle@kyleam.com> wrote:
> It seems like org-babel-execute already covers this processing.

s/org-babel-execute/org-babel-execute:ruby/

--
Kyle

Re: fa5fd6351605912ec75e783cb62649 breaks org-babel-script-escape for ob-ruby

[Matthew MacLean]

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

Yeah, my initial patch was actually for ob-ruby, though, when looking for
the change that broke it for the report, I found a change in behaviour of
the escaping function and figured that Ruby might not be the only thing
broken.

So, instead, I restored the original permissive behaviour of
org-babel-script-escape and made a patch of that instead.

On Tue, Aug 11, 2015, 4:36 PM Kyle Meyer <kyle@kyleam.com> wrote:

> Kyle Meyer <kyle@kyleam.com> wrote:
> > It seems like org-babel-execute already covers this processing.
>
> s/org-babel-execute/org-babel-execute:ruby/
>
> --
> Kyle
>

[-- Attachment #2: Type: text/html, Size: 922 bytes --]

Re: fa5fd6351605912ec75e783cb62649 breaks org-babel-script-escape for ob-ruby

[Kyle Meyer]

Matthew MacLean <archenoth@gmail.com> wrote:
> Yeah, my initial patch was actually for ob-ruby, though, when looking for
> the change that broke it for the report, I found a change in behaviour of
> the escaping function and figured that Ruby might not be the only thing
> broken.

Yes, that makes sense, but glancing at all lisp/ob-*.el files that use
org-babel-script-escape, ob-ruby.el seems to be the only one that makes
a double call to org-babel-script-escape, so I'd guess it is being
called with a string in all other cases.

> So, instead, I restored the original permissive behaviour of
> org-babel-script-escape and made a patch of that instead.

Given that org-babel-script-escape gives a clear error message, I'd
prefer to keep the stricter variant and fix any offending calls.

Feel free to wait to hear others' thoughts on this, but, in any case,
please use 'git format-patch' to generate a patch with a commit message
(see the contributing section of the website [1] for more information).
Also, it'd be nice to add a test that catches the original error.

Thanks

[1] http://orgmode.org/worg/org-contribute.html

--
Kyle

Re: fa5fd6351605912ec75e783cb62649 breaks org-babel-script-escape for ob-ruby

[Matthew MacLean]

[-- Attachment #1.1: Type: text/plain, Size: 898 bytes --]

If the stricter definition covers everything that org-babel-escape-script
was supposed to do... I agree we should keep it.
My problem was I wasn't sure if the function got pruned of something it
needed.

But if that's not the case...

On Tue, Aug 11, 2015 at 6:52 PM, Kyle Meyer <kyle@kyleam.com> wrote:

> Feel free to wait to hear others' thoughts on this, but, in any case,
> please use 'git format-patch' to generate a patch with a commit message
> (see the contributing section of the website [1] for more information).
> Also, it'd be nice to add a test that catches the original error.
>

...here is the git format-patch of the ob-ruby.el change!

I also created one for a test to check for basic Ruby evaluation
capabilities. It should catch errors that break evaluation like this in
ob-ruby. (I find it interesting that the session tests worked without the
double-escape change actually.)

[-- Attachment #1.2: Type: text/html, Size: 1449 bytes --]

[-- Attachment #2: 0001-Removed-double-escape-in-ob-ruby.el-In-org-babel-rub.patch --]
[-- Type: text/x-patch, Size: 1320 bytes --]

From 07052b4ad0f51f24534c5ee2899511cfb658ae71 Mon Sep 17 00:00:00 2001
From: Archenoth <archenoth@gmail.com>
Date: Tue, 11 Aug 2015 20:29:53 -0600
Subject: [PATCH 1/2] Removed double-escape in ob-ruby.el (In
 org-babel-ruby-evaluate)

The only time org-babel-ruby-evaluate is called is in
org-babel-execute:ruby, where its result either escaped (The double
escape) or passed in as the "scalar-form" of org-babel-result-cond which
handles the "pp" and "code" parameters. (So, places that don't need
escaping.)
---
 lisp/ob-ruby.el | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/lisp/ob-ruby.el b/lisp/ob-ruby.el
index 9b01dbf..0ff460e 100644
--- a/lisp/ob-ruby.el
+++ b/lisp/ob-ruby.el
@@ -201,11 +201,7 @@ return the value of the last statement in BODY, as elisp."
 			      org-babel-ruby-pp-wrapper-method
 			    org-babel-ruby-wrapper-method)
 			  body (org-babel-process-file-name tmp-file 'noquote)))
-		 (let ((raw (org-babel-eval-read-file tmp-file)))
-                   (if (or (member "code" result-params)
-                           (member "pp" result-params))
-                       raw
-                     (org-babel-ruby-table-or-string raw))))))
+		 (org-babel-eval-read-file tmp-file))))
     ;; comint session evaluation
     (case result-type
       (output
-- 
2.5.0


[-- Attachment #3: 0002-Add-test-that-basic-Ruby-evaluation-works-with-ob-ru.patch --]
[-- Type: text/x-patch, Size: 1152 bytes --]

From 40864852d9f0996c4f755377a8326f2cbae417ec Mon Sep 17 00:00:00 2001
From: Archenoth <archenoth@gmail.com>
Date: Tue, 11 Aug 2015 21:18:44 -0600
Subject: [PATCH 2/2] Add test that basic Ruby evaluation works with ob-ruby

This will also test that the string escape schenanigans are fixed.
---
 testing/lisp/test-ob-ruby.el | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/testing/lisp/test-ob-ruby.el b/testing/lisp/test-ob-ruby.el
index eb5233b..576cb13 100644
--- a/testing/lisp/test-ob-ruby.el
+++ b/testing/lisp/test-ob-ruby.el
@@ -21,6 +21,23 @@
 (unless (featurep 'ob-ruby)
   (signal 'missing-test-dependency "Support for Ruby code blocks"))
 
+(ert-deftest test-ob-ruby/basic-evaluation ()
+  "Test that basic evaluation works."
+    (should (equal (org-test-with-temp-text "#+begin_src ruby
+ 2 + 2
+#+end_src"
+  (org-babel-execute-maybe)
+  (substring-no-properties
+   (buffer-string)))
+		   "#+begin_src ruby
+ 2 + 2
+#+end_src
+
+#+RESULTS:
+: 4
+
+")))
+
 (ert-deftest test-ob-ruby/session-output-1 ()
     (should (equal (org-test-with-temp-text "#+begin_src ruby :session :results output
 s = \"1\"
-- 
2.5.0

Re: fa5fd6351605912ec75e783cb62649 breaks org-babel-script-escape for ob-ruby

[Kyle Meyer]

Matthew MacLean <archenoth@gmail.com> wrote:
[...]
> ...here is the git format-patch of the ob-ruby.el change!

> I also created one for a test to check for basic Ruby evaluation
> capabilities. It should catch errors that break evaluation like this in
> ob-ruby.

Thanks.  The code change looks good to me and fixes the provided test.

Please format the commit message like described here:

  http://orgmode.org/worg/org-contribute.html#orgheadline6

In addition to the formatting, the message should include "TINYCHANGE".
This may be beyond what qualifies as a tiny change if tests count toward
changed lines (Bastien or Nicolas?).  If it's acceptable as a tiny
change, please combine the two commits.  Otherwise, perhaps we can just
leave the test commit out.

> (I find it interesting that the session tests worked without the
> double-escape change actually.)

The session tests never execute a branch that calls
org-babel-ruby-table-or-string.

--
Kyle

Re: fa5fd6351605912ec75e783cb62649 breaks org-babel-script-escape for ob-ruby

[Matthew MacLean]

[-- Attachment #1.1: Type: text/plain, Size: 495 bytes --]

On Tue, Aug 11, 2015 at 11:53 PM, Kyle Meyer <kyle@kyleam.com> wrote:

> In addition to the formatting, the message should include "TINYCHANGE".
> This may be beyond what qualifies as a tiny change if tests count toward
> changed lines (Bastien or Nicolas?).  If it's acceptable as a tiny
> change, please combine the two commits.  Otherwise, perhaps we can just
> leave the test commit out.
>


Alright, done. Is this acceptable? (Provided that tests don't count towards
line count, of course)

[-- Attachment #1.2: Type: text/html, Size: 910 bytes --]

[-- Attachment #2: 0001-ob-ruby-Fix-double-escaping.patch --]
[-- Type: text/x-patch, Size: 2477 bytes --]

From 7c849019341805e8814efecb11ea5bc1bd2a47e5 Mon Sep 17 00:00:00 2001
From: Archenoth <archenoth@gmail.com>
Date: Tue, 11 Aug 2015 23:59:25 -0600
Subject: [PATCH] ob-ruby: Fix double-escaping

* lisp/ob-ruby.el: Remove second call to
  `org-babel-ruby-table-or-string' in `org-babel-ruby-evaluate'.

* testing/lisp/test-ob-ruby.el: Add test to verify
  `org-babel-execute:ruby' can evaluate Ruby code. (What the
  double-escape prevented)

I removed the escaping from `org-babel-ruby-evaluate', because the only
place `org-babel-ruby-evaluate' is ever called is in
`org-babel-execute:ruby'.

In this function, its result either escaped (Where the double escape
previously occurred) or passed in as the "scalar-form" of
`org-babel-result-cond', which handles the "pp" and "code" parameters.
(A place that doesn't need escaping.)

TINYCHANGE
---
 lisp/ob-ruby.el              |  6 +-----
 testing/lisp/test-ob-ruby.el | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/lisp/ob-ruby.el b/lisp/ob-ruby.el
index 9b01dbf..0ff460e 100644
--- a/lisp/ob-ruby.el
+++ b/lisp/ob-ruby.el
@@ -201,11 +201,7 @@ return the value of the last statement in BODY, as elisp."
 			      org-babel-ruby-pp-wrapper-method
 			    org-babel-ruby-wrapper-method)
 			  body (org-babel-process-file-name tmp-file 'noquote)))
-		 (let ((raw (org-babel-eval-read-file tmp-file)))
-                   (if (or (member "code" result-params)
-                           (member "pp" result-params))
-                       raw
-                     (org-babel-ruby-table-or-string raw))))))
+		 (org-babel-eval-read-file tmp-file))))
     ;; comint session evaluation
     (case result-type
       (output
diff --git a/testing/lisp/test-ob-ruby.el b/testing/lisp/test-ob-ruby.el
index eb5233b..576cb13 100644
--- a/testing/lisp/test-ob-ruby.el
+++ b/testing/lisp/test-ob-ruby.el
@@ -21,6 +21,23 @@
 (unless (featurep 'ob-ruby)
   (signal 'missing-test-dependency "Support for Ruby code blocks"))

+(ert-deftest test-ob-ruby/basic-evaluation ()
+  "Test that basic evaluation works."
+    (should (equal (org-test-with-temp-text "#+begin_src ruby
+ 2 + 2
+#+end_src"
+  (org-babel-execute-maybe)
+  (substring-no-properties
+   (buffer-string)))
+		   "#+begin_src ruby
+ 2 + 2
+#+end_src
+
+#+RESULTS:
+: 4
+
+")))
+
 (ert-deftest test-ob-ruby/session-output-1 ()
     (should (equal (org-test-with-temp-text "#+begin_src ruby :session :results output
 s = \"1\"
--
2.5.0

Re: fa5fd6351605912ec75e783cb62649 breaks org-babel-script-escape for ob-ruby

[Kyle Meyer]

Matthew MacLean <archenoth@gmail.com> writes:

> Alright, done. Is this acceptable? (Provided that tests don't count towards
> line count, of course)

Thanks.  A few minor comments on the commit message.

> Subject: [PATCH] ob-ruby: Fix double-escaping
>
> * lisp/ob-ruby.el: Remove second call to
>   `org-babel-ruby-table-or-string' in `org-babel-ruby-evaluate'.

Please add the name of the changed function in parentheses after the
file name rather than putting it in the description body.

> * testing/lisp/test-ob-ruby.el: Add test to verify
>   `org-babel-execute:ruby' can evaluate Ruby code. (What the
>   double-escape prevented)

Same here for the test name.  "Add test." for description would do.

> I removed the escaping from `org-babel-ruby-evaluate', because the only
> place `org-babel-ruby-evaluate' is ever called is in
> `org-babel-execute:ruby'.
>
> In this function, its result either escaped (Where the double escape
> previously occurred) or passed in as the "scalar-form" of
> `org-babel-result-cond', which handles the "pp" and "code" parameters.
> (A place that doesn't need escaping.)

I think the above two paragraphs could be replaced by a link to this ML
post.

Thanks for working on this.

--
Kyle

Re: fa5fd6351605912ec75e783cb62649 breaks org-babel-script-escape for ob-ruby

[Matthew MacLean]

[-- Attachment #1.1: Type: text/plain, Size: 1553 bytes --]

In that case... Here is another patch with your suggestions.

Thanks for taking the time to point out all that..! I'll be sure to keep it
all in mind if I submit something else later.

On Wed, Aug 12, 2015 at 11:21 AM, Kyle Meyer <kyle@kyleam.com> wrote:

> Matthew MacLean <archenoth@gmail.com> writes:
>
> > Alright, done. Is this acceptable? (Provided that tests don't count
> towards
> > line count, of course)
>
> Thanks.  A few minor comments on the commit message.
>
> > Subject: [PATCH] ob-ruby: Fix double-escaping
> >
> > * lisp/ob-ruby.el: Remove second call to
> >   `org-babel-ruby-table-or-string' in `org-babel-ruby-evaluate'.
>
> Please add the name of the changed function in parentheses after the
> file name rather than putting it in the description body.
>
> > * testing/lisp/test-ob-ruby.el: Add test to verify
> >   `org-babel-execute:ruby' can evaluate Ruby code. (What the
> >   double-escape prevented)
>
> Same here for the test name.  "Add test." for description would do.
>
> > I removed the escaping from `org-babel-ruby-evaluate', because the only
> > place `org-babel-ruby-evaluate' is ever called is in
> > `org-babel-execute:ruby'.
> >
> > In this function, its result either escaped (Where the double escape
> > previously occurred) or passed in as the "scalar-form" of
> > `org-babel-result-cond', which handles the "pp" and "code" parameters.
> > (A place that doesn't need escaping.)
>
> I think the above two paragraphs could be replaced by a link to this ML
> post.
>
> Thanks for working on this.
>
> --
> Kyle
>

[-- Attachment #1.2: Type: text/html, Size: 2189 bytes --]

[-- Attachment #2: 0001-ob-ruby-Fix-double-escaping.patch --]
[-- Type: text/x-patch, Size: 2073 bytes --]

From 17447319eee53fbfcc6123c384842dd7fd04e3b8 Mon Sep 17 00:00:00 2001
From: Archenoth <archenoth@gmail.com>
Date: Tue, 11 Aug 2015 23:59:25 -0600
Subject: [PATCH] ob-ruby: Fix double-escaping

* lisp/ob-ruby.el (org-babel-ruby-evaluate): Remove second call to
  `org-babel-ruby-table-or-string'.

* testing/lisp/test-ob-ruby.el (org-babel-ruby-evaluate): Add test.

<http://permalink.gmane.org/gmane.emacs.orgmode/99888>

TINYCHANGE
---
 lisp/ob-ruby.el              |  6 +-----
 testing/lisp/test-ob-ruby.el | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/lisp/ob-ruby.el b/lisp/ob-ruby.el
index 9b01dbf..0ff460e 100644
--- a/lisp/ob-ruby.el
+++ b/lisp/ob-ruby.el
@@ -201,11 +201,7 @@ return the value of the last statement in BODY, as elisp."
 			      org-babel-ruby-pp-wrapper-method
 			    org-babel-ruby-wrapper-method)
 			  body (org-babel-process-file-name tmp-file 'noquote)))
-		 (let ((raw (org-babel-eval-read-file tmp-file)))
-                   (if (or (member "code" result-params)
-                           (member "pp" result-params))
-                       raw
-                     (org-babel-ruby-table-or-string raw))))))
+		 (org-babel-eval-read-file tmp-file))))
     ;; comint session evaluation
     (case result-type
       (output
diff --git a/testing/lisp/test-ob-ruby.el b/testing/lisp/test-ob-ruby.el
index eb5233b..576cb13 100644
--- a/testing/lisp/test-ob-ruby.el
+++ b/testing/lisp/test-ob-ruby.el
@@ -21,6 +21,23 @@
 (unless (featurep 'ob-ruby)
   (signal 'missing-test-dependency "Support for Ruby code blocks"))
 
+(ert-deftest test-ob-ruby/basic-evaluation ()
+  "Test that basic evaluation works."
+    (should (equal (org-test-with-temp-text "#+begin_src ruby
+ 2 + 2
+#+end_src"
+  (org-babel-execute-maybe)
+  (substring-no-properties
+   (buffer-string)))
+		   "#+begin_src ruby
+ 2 + 2
+#+end_src
+
+#+RESULTS:
+: 4
+
+")))
+
 (ert-deftest test-ob-ruby/session-output-1 ()
     (should (equal (org-test-with-temp-text "#+begin_src ruby :session :results output
 s = \"1\"
-- 
2.5.0

Re: fa5fd6351605912ec75e783cb62649 breaks org-babel-script-escape for ob-ruby

[Kyle Meyer]

Matthew MacLean <archenoth@gmail.com> writes:

> In that case... Here is another patch with your suggestions.

I've pushed this (stripping out the test).  Thank you for the fix.

--
Kyle

Re: fa5fd6351605912ec75e783cb62649 breaks org-babel-script-escape for ob-ruby

[Matthew MacLean]

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

Gnarly--and thank you for the help!

On Wed, Aug 12, 2015 at 9:41 PM, Kyle Meyer <kyle@kyleam.com> wrote:

> Matthew MacLean <archenoth@gmail.com> writes:
>
> > In that case... Here is another patch with your suggestions.
>
> I've pushed this (stripping out the test).  Thank you for the fix.
>
> --
> Kyle
>

[-- Attachment #2: Type: text/html, Size: 705 bytes --]