[PATCH] ob-java.el: Allow for more whitespace

[PATCH] ob-java.el: Allow for more whitespace

[ian martins]

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

Two patches are attached. One allows for more whitespace in java code
blocks. The other fixes a bug that would result in a main method being
wrapped in another main method.

[-- Attachment #2: 0001-ob-java.el-Do-not-wrap-a-main-method-in-a-main-metho.patch --]
[-- Type: text/x-patch, Size: 1346 bytes --]

From 9fbb5a436ebc007d19adc20fcee7ebf08e4aec3b Mon Sep 17 00:00:00 2001
From: Ian Martins <ianxm@jhu.edu>
Date: Thu, 12 Nov 2020 05:09:11 -0500
Subject: [PATCH 1/2] ob-java.el: Do not wrap a main method in a main method

* lisp/ob-java.el (org-babel-expand-body:java): The code was checking
for existence of a class declaration before wrapping the content of
the code block in a main method, but it should be checking for
existence of a main method.
---
 lisp/ob-java.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/ob-java.el b/lisp/ob-java.el
index d8d4db852..92e873f0d 100644
--- a/lisp/ob-java.el
+++ b/lisp/ob-java.el
@@ -296,7 +296,7 @@ is simplest to expand the code block from the inside out."
       ;; wrap main.  If there are methods defined, but no main method
       ;; and no class, wrap everything in a generic main method.
       (goto-char (point-min))
-      (when (and (not (re-search-forward org-babel-java--class-re nil t))
+      (when (and (not (re-search-forward org-babel-java--main-re nil t))
                  (not (re-search-forward org-babel-java--any-method-re nil t)))
         (org-babel-java--move-past org-babel-java--package-re) ; if package is defined, move past it
         (org-babel-java--move-past org-babel-java--imports-re) ; if imports are defined, move past them
-- 
2.25.1


[-- Attachment #3: 0002-ob-java.el-Allow-for-more-whitespace-in-java-code.patch --]
[-- Type: text/x-patch, Size: 3218 bytes --]

From 19af495f4152cd7b8580ceab2a168182f44bedbb Mon Sep 17 00:00:00 2001
From: Ian Martins <ianxm@jhu.edu>
Date: Thu, 12 Nov 2020 05:18:48 -0500
Subject: [PATCH 2/2] ob-java.el: Allow for more whitespace in java code

* lisp/ob-java.el (defconst *-re): Updated regexps to allow for more
whitespace in the content of java code blocks, and removed some
redundancies.

* testing/lisp/test-ob-java.el (ob-java/simple-with-main-whitespace):
Added test case with lots of whitespace.
---
 lisp/ob-java.el              | 10 +++++-----
 testing/lisp/test-ob-java.el | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/lisp/ob-java.el b/lisp/ob-java.el
index 92e873f0d..d65176d4e 100644
--- a/lisp/ob-java.el
+++ b/lisp/ob-java.el
@@ -77,15 +77,15 @@ like javac -verbose."
   :package-version '(Org . "9.5")
   :type 'symbol)
 
-(defconst org-babel-java--package-re "^[[:space:]]*package[[:space:]]+\\\([[:alnum:]_\.]+\\\);$"
+(defconst org-babel-java--package-re "^[[:space:]]*package[[:space:]]+\\\([[:alnum:]_\.]+\\\)[[:space:]]*;$"
   "Regexp for the package statement.")
-(defconst org-babel-java--imports-re "^[[:space:]]*import[[:space:]]+\\\([[:alnum:]_\.]+\\\);$"
+(defconst org-babel-java--imports-re "^[[:space:]]*import[[:space:]]+\\\([[:alnum:]_\.]+\\\)[[:space:]]*;$"
   "Regexp for import statements.")
-(defconst org-babel-java--class-re "^[[:space:]]*\\\(?:public[[:space:]]+\\\)?class[[:space:]]+\\\([[:alnum:]_]+\\\)[[:space:]]*\n?[[:space:]]*{"
+(defconst org-babel-java--class-re "^[[:space:]]*\\\(?:public[[:space:]]+\\\)?class[[:space:]]+\\\([[:alnum:]_]+\\\)[[:space:]]*{"
   "Regexp for the class declaration.")
-(defconst org-babel-java--main-re "public static void main(String\\\(?:\\[]\\\)?[[:space:]]+[^ ]+\\\(?:\\[]\\\)?).*\n?[[:space:]]*{"
+(defconst org-babel-java--main-re "public[[:space:]]+static[[:space:]]+void[[:space:]]+main[[:space:]]*([[:space:]]*String[[:space:]]*.*[[:space:]]*)[[:space:]]*.*[[:space:]]*{"
   "Regexp for the main method declaration.")
-(defconst org-babel-java--any-method-re "public .*(.*).*\n?[[:space:]]*{"
+(defconst org-babel-java--any-method-re "public[[:space:]]+.*[[:space:]]*([[:space:]]*.*[[:space:]]*)[[:space:]]*.*[[:space:]]*{"
   "Regexp for any method.")
 (defconst org-babel-java--result-wrapper "\n    public static String __toString(Object val) {
         if (val instanceof String) {
diff --git a/testing/lisp/test-ob-java.el b/testing/lisp/test-ob-java.el
index e14975412..50d3ef5b0 100644
--- a/testing/lisp/test-ob-java.el
+++ b/testing/lisp/test-ob-java.el
@@ -128,6 +128,24 @@ public static void main(String args[]) {
 #+end_src"
     (should (string= "42" (org-babel-execute-src-block)))))
 
+(ert-deftest ob-java/simple-with-main-whitespace ()
+  "Hello world program that defines a main function with the square brackets after `args'."
+  (org-test-with-temp-text
+      "#+begin_src java :results output silent
+public
+static
+void
+main
+ (
+ String
+ args []
+ )
+{
+    System.out.print(42);
+}
+#+end_src"
+    (should (string= "42" (org-babel-execute-src-block)))))
+
 (ert-deftest ob-java/simple-with-class ()
   "Hello world program that defines a class."
   (org-test-with-temp-text
-- 
2.25.1

Re: [PATCH] ob-java.el: Allow for more whitespace

[Kyle Meyer]

ian martins writes:

> Subject: [PATCH 1/2] ob-java.el: Do not wrap a main method in a main method
>
> * lisp/ob-java.el (org-babel-expand-body:java): The code was checking
> for existence of a class declaration before wrapping the content of
> the code block in a main method, but it should be checking for
> existence of a main method.

Sounds good.

> Subject: [PATCH 2/2] ob-java.el: Allow for more whitespace in java code
>
> * lisp/ob-java.el (defconst *-re): Updated regexps to allow for more
> whitespace in the content of java code blocks, and removed some
> redundancies.

Sorry, more change log nitpicking from me (which is even less fun to do
than other nitpicking because I dislike the practice of including change
logs in commit messages).

Please name each variable in full.  Here's the relevant bit from the
guidelines that Emacs's CONTRIBUTE points to:

    If you mention the names of the modified functions or variables,
    it’s important to name them in full.  Don’t abbreviate function or
    variable names, and don’t combine them.  Subsequent maintainers will
    often search for a function name to find all the change log entries
    that pertain to it; if you abbreviate the name, they won’t find it
    when they search.

https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html#Style-of-Change-Logs

We should probably link to that in worg's org-contribute.org.

> * testing/lisp/test-ob-java.el (ob-java/simple-with-main-whitespace):
> Added test case with lots of whitespace.

Is this related to Jarmo's report at
<https://orgmode.org/list/87o8k68w05.fsf@iki.fi>?  If so, it'd be good
to include a Reported-by trailer as well as a link.

> -(defconst org-babel-java--package-re "^[[:space:]]*package[[:space:]]+\\\([[:alnum:]_\.]+\\\);$"
> +(defconst org-babel-java--package-re "^[[:space:]]*package[[:space:]]+\\\([[:alnum:]_\.]+\\\)[[:space:]]*;$"
>    "Regexp for the package statement.")
> -(defconst org-babel-java--imports-re "^[[:space:]]*import[[:space:]]+\\\([[:alnum:]_\.]+\\\);$"
> +(defconst org-babel-java--imports-re "^[[:space:]]*import[[:space:]]+\\\([[:alnum:]_\.]+\\\)[[:space:]]*;$"
>    "Regexp for import statements.")
> -(defconst org-babel-java--class-re "^[[:space:]]*\\\(?:public[[:space:]]+\\\)?class[[:space:]]+\\\([[:alnum:]_]+\\\)[[:space:]]*\n?[[:space:]]*{"
> +(defconst org-babel-java--class-re "^[[:space:]]*\\\(?:public[[:space:]]+\\\)?class[[:space:]]+\\\([[:alnum:]_]+\\\)[[:space:]]*{"
>    "Regexp for the class declaration.")
> -(defconst org-babel-java--main-re "public static void main(String\\\(?:\\[]\\\)?[[:space:]]+[^ ]+\\\(?:\\[]\\\)?).*\n?[[:space:]]*{"
> +(defconst org-babel-java--main-re "public[[:space:]]+static[[:space:]]+void[[:space:]]+main[[:space:]]*([[:space:]]*String[[:space:]]*.*[[:space:]]*)[[:space:]]*.*[[:space:]]*{"
>    "Regexp for the main method declaration.")
> -(defconst org-babel-java--any-method-re "public .*(.*).*\n?[[:space:]]*{"
> +(defconst org-babel-java--any-method-re "public[[:space:]]+.*[[:space:]]*([[:space:]]*.*[[:space:]]*)[[:space:]]*.*[[:space:]]*{"
>    "Regexp for any method.")

Not speaking Java, I don't have anything actually valuable to say about
this change, but I wouldn't complain if these regular expressions were
switched over to rx (or at least tamed a bit in terms of line length).

Thanks for the fixes.

Re: [PATCH] ob-java.el: Allow for more whitespace

[ian martins]

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

On Thu, Nov 12, 2020 at 10:46 PM Kyle Meyer <kyle@kyleam.com> wrote:
>
> ian martins writes:
>
> > Subject: [PATCH 2/2] ob-java.el: Allow for more whitespace in java code
> >
> > * lisp/ob-java.el (defconst *-re): Updated regexps to allow for more
> > whitespace in the content of java code blocks, and removed some
> > redundancies.
>
> Sorry, more change log nitpicking from me (which is even less fun to do
> than other nitpicking because I dislike the practice of including change
> logs in commit messages).

No problem. Gathering the list of changed names is easy (I use emacs).
I thought the long list would make the entry less useful, but I can
see how it makes it more searchable this way. Of course, people could
search the code diffs instead and then the commit logs could just be
written for people.

> Please name each variable in full.  Here's the relevant bit from the
> guidelines that Emacs's CONTRIBUTE points to:
>
>     If you mention the names of the modified functions or variables,
>     it’s important to name them in full.  Don’t abbreviate function or
>     variable names, and don’t combine them.  Subsequent maintainers will
>     often search for a function name to find all the change log entries
>     that pertain to it; if you abbreviate the name, they won’t find it
>     when they search.
>
> https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html#Style-of-Change-Logs
>
> We should probably link to that in worg's org-contribute.org.

Thanks for providing the reference. I'll add a link to worg if there isn't one.

> > * testing/lisp/test-ob-java.el (ob-java/simple-with-main-whitespace):
> > Added test case with lots of whitespace.
>
> Is this related to Jarmo's report at
> <https://orgmode.org/list/87o8k68w05.fsf@iki.fi>?  If so, it'd be good
> to include a Reported-by trailer as well as a link.

Yes. The updated patch includes Reported-by. That is just text in the
commit message, not a git option, right?

> > -(defconst org-babel-java--package-re "^[[:space:]]*package[[:space:]]+\\\([[:alnum:]_\.]+\\\);$"
> > +(defconst org-babel-java--package-re "^[[:space:]]*package[[:space:]]+\\\([[:alnum:]_\.]+\\\)[[:space:]]*;$"
> >    "Regexp for the package statement.")
> > -(defconst org-babel-java--imports-re "^[[:space:]]*import[[:space:]]+\\\([[:alnum:]_\.]+\\\);$"
> > +(defconst org-babel-java--imports-re "^[[:space:]]*import[[:space:]]+\\\([[:alnum:]_\.]+\\\)[[:space:]]*;$"
> >    "Regexp for import statements.")
> > -(defconst org-babel-java--class-re "^[[:space:]]*\\\(?:public[[:space:]]+\\\)?class[[:space:]]+\\\([[:alnum:]_]+\\\)[[:space:]]*\n?[[:space:]]*{"
> > +(defconst org-babel-java--class-re "^[[:space:]]*\\\(?:public[[:space:]]+\\\)?class[[:space:]]+\\\([[:alnum:]_]+\\\)[[:space:]]*{"
> >    "Regexp for the class declaration.")
> > -(defconst org-babel-java--main-re "public static void main(String\\\(?:\\[]\\\)?[[:space:]]+[^ ]+\\\(?:\\[]\\\)?).*\n?[[:space:]]*{"
> > +(defconst org-babel-java--main-re "public[[:space:]]+static[[:space:]]+void[[:space:]]+main[[:space:]]*([[:space:]]*String[[:space:]]*.*[[:space:]]*)[[:space:]]*.*[[:space:]]*{"
> >    "Regexp for the main method declaration.")
> > -(defconst org-babel-java--any-method-re "public .*(.*).*\n?[[:space:]]*{"
> > +(defconst org-babel-java--any-method-re "public[[:space:]]+.*[[:space:]]*([[:space:]]*.*[[:space:]]*)[[:space:]]*.*[[:space:]]*{"
> >    "Regexp for any method.")
>
> Not speaking Java, I don't have anything actually valuable to say about
> this change, but I wouldn't complain if these regular expressions were
> switched over to rx (or at least tamed a bit in terms of line length).

Thanks for the suggestion. I hadn't seen `rx' before. It's beautiful.
I converted it. That was a joy.

[-- Attachment #2: 0002-ob-java.el-Allow-for-more-whitespace-in-java-code.patch --]
[-- Type: text/x-patch, Size: 4380 bytes --]

From 4784ea1e926da014e30bbbaa241b3779a14119f4 Mon Sep 17 00:00:00 2001
From: Ian Martins <ianxm@jhu.edu>
Date: Thu, 12 Nov 2020 05:18:48 -0500
Subject: [PATCH 2/2] ob-java.el: Allow for more whitespace in java code

* lisp/ob-java.el (org-babel-java--package-re)
(org-babel-java--imports-re, org-babel-java--class-re)
(org-babel-java--main-re, org-babel-java--any-method-re):
Updated regexps to allow for more whitespace in the content of java
code blocks.  Convert regexps to `rx' to improve clarity.
* testing/lisp/test-ob-java.el (ob-java/simple-with-main-whitespace):
Added test case with excessive whitespace.

Reported-by: Jarmo Hurri <jarmo.hurri@iki.fi>
<https://orgmode.org/list/87o8k68w05.fsf@iki.fi>
---
 lisp/ob-java.el              | 35 ++++++++++++++++++++++++++++++-----
 testing/lisp/test-ob-java.el | 18 ++++++++++++++++++
 2 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/lisp/ob-java.el b/lisp/ob-java.el
index 92e873f0d..4cf80433f 100644
--- a/lisp/ob-java.el
+++ b/lisp/ob-java.el
@@ -77,15 +77,40 @@ like javac -verbose."
   :package-version '(Org . "9.5")
   :type 'symbol)
 
-(defconst org-babel-java--package-re "^[[:space:]]*package[[:space:]]+\\\([[:alnum:]_\.]+\\\);$"
+(defconst org-babel-java--package-re (rx line-start (0+ space) "package"
+					 (1+ space) (group (1+ (in alnum ?_ ?.))) ; capture the package name
+					 (0+ space) ?\; line-end)
   "Regexp for the package statement.")
-(defconst org-babel-java--imports-re "^[[:space:]]*import[[:space:]]+\\\([[:alnum:]_\.]+\\\);$"
+(defconst org-babel-java--imports-re (rx line-start (0+ space) "import"
+					 (1+ space) (group (1+ (in alnum ?_ ?.))) ; capture the fully qualified class name
+					 (0+ space) ?\; line-end)
   "Regexp for import statements.")
-(defconst org-babel-java--class-re "^[[:space:]]*\\\(?:public[[:space:]]+\\\)?class[[:space:]]+\\\([[:alnum:]_]+\\\)[[:space:]]*\n?[[:space:]]*{"
+(defconst org-babel-java--class-re (rx line-start (0+ space) (opt (seq "public" (1+ space)))
+				       "class" (1+ space)
+				       (group (1+ (in alnum ?_))) ; capture the class name
+				       (0+ space) ?{)
   "Regexp for the class declaration.")
-(defconst org-babel-java--main-re "public static void main(String\\\(?:\\[]\\\)?[[:space:]]+[^ ]+\\\(?:\\[]\\\)?).*\n?[[:space:]]*{"
+(defconst org-babel-java--main-re (rx line-start (0+ space) "public"
+				      (1+ space) "static"
+				      (1+ space) "void"
+				      (1+ space) "main"
+				      (0+ space) ?\(
+				      (0+ space) "String"
+				      (0+ space) (1+ (in alnum ?_ ?\[ ?\] space)) ; "[] args" or "args[]"
+				      (0+ space) ?\)
+				      (0+ space) (opt "throws" (1+ (in alnum ?_ ?, ?. space)))
+				      ?{)
   "Regexp for the main method declaration.")
-(defconst org-babel-java--any-method-re "public .*(.*).*\n?[[:space:]]*{"
+(defconst org-babel-java--any-method-re (rx line-start
+					    (0+ space) (opt (seq (1+ alnum) (1+ space)))   ; visibility
+					    (opt (seq "static" (1+ space)))                ; binding
+					    (1+ (in alnum ?_ ?\[ ?\]))                     ; return type
+                                            (1+ space) (1+ (in alnum ?_))                  ; method name
+					    (0+ space) ?\(
+					    (0+ space) (0+ (in alnum ?_ ?\[ ?\] ?, space)) ; params
+					    (0+ space) ?\)
+					    (0+ space) (opt "throws" (1+ (in alnum ?_ ?, ?. space)))
+					    ?{)
   "Regexp for any method.")
 (defconst org-babel-java--result-wrapper "\n    public static String __toString(Object val) {
         if (val instanceof String) {
diff --git a/testing/lisp/test-ob-java.el b/testing/lisp/test-ob-java.el
index e14975412..50d3ef5b0 100644
--- a/testing/lisp/test-ob-java.el
+++ b/testing/lisp/test-ob-java.el
@@ -128,6 +128,24 @@ public static void main(String args[]) {
 #+end_src"
     (should (string= "42" (org-babel-execute-src-block)))))
 
+(ert-deftest ob-java/simple-with-main-whitespace ()
+  "Hello world program that defines a main function with the square brackets after `args'."
+  (org-test-with-temp-text
+      "#+begin_src java :results output silent
+public
+static
+void
+main
+ (
+ String
+ args []
+ )
+{
+    System.out.print(42);
+}
+#+end_src"
+    (should (string= "42" (org-babel-execute-src-block)))))
+
 (ert-deftest ob-java/simple-with-class ()
   "Hello world program that defines a class."
   (org-test-with-temp-text
-- 
2.25.1

Re: [PATCH] ob-java.el: Allow for more whitespace

[Kyle Meyer]

ian martins writes:

> On Thu, Nov 12, 2020 at 10:46 PM Kyle Meyer <kyle@kyleam.com> wrote:

>> Is this related to Jarmo's report at
>> <https://orgmode.org/list/87o8k68w05.fsf@iki.fi>?  If so, it'd be good
>> to include a Reported-by trailer as well as a link.
>
> Yes. The updated patch includes Reported-by. That is just text in the
> commit message, not a git option, right?

Correct, it's just the block at the end of the commit message.  If you
use Magit's git-commit.el, there are some helpers for inserting common
trailers, including git-commit-reported ('C-c C-p').

The main thing is that the information is there in some form, so
everything that follows next is very much in the "doesn't really matter"
territory...

I've recently tried to be more consistent with the format I use.  In
particular, giving the link a keyword rather than putting a plain value
underneath:

  Reported-by: Name <email>
  Ref: link

My reason for this is that Git does have a concept of what a trailer is
(see 'man git-interpret-trailers').  Without the "Ref:" (or some other
keyword), the link is seen as part of the Reported-by value if the link
has indentation.  Without indentation, the Reported-by is no longer seen
as a trailer.

And with Git 2.29, I've become a little more interested in Git being
able to parse these because shortlog learned "--group=trailer:".  For
example

  $ git shortlog -s --group=trailer:reported-by release_9.4.. | head -3
       1	Allen Li
       1	B Goodr
       1	Charles Tam


I think that's kind of neat, but again doesn't really matter :)

Thanks for the updates.

Re: [PATCH] ob-java.el: Allow for more whitespace

[Bastien]

ian martins <ianxm@jhu.edu> writes:

> Two patches are attached. One allows for more whitespace in java code
> blocks. The other fixes a bug that would result in a main method being
> wrapped in another main method.

Closing this now, thanks!

-- 
 Bastien