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

[ian martins <ianxm@jhu.edu>]

[-- 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