From: ian martins <ianxm@jhu.edu>
To: Kyle Meyer <kyle@kyleam.com>
Cc: Org-Mode mailing list <emacs-orgmode@gnu.org>
Subject: Re: [PATCH] ob-java.el: Allow for more whitespace
Date: Sat, 14 Nov 2020 04:40:28 -0500 [thread overview]
Message-ID: <CAC=rjb7G2noq9vm_-LVRN-BLu9uO2+So4A0Np_n6m3i=dfHt=Q@mail.gmail.com> (raw)
In-Reply-To: <87d00h7wbr.fsf@kyleam.com>
[-- 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
next prev parent reply other threads:[~2020-11-14 9:41 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-12 10:38 [PATCH] ob-java.el: Allow for more whitespace ian martins
2020-11-13 3:46 ` Kyle Meyer
2020-11-14 9:40 ` ian martins [this message]
2020-11-14 18:21 ` Kyle Meyer
2020-12-14 6:11 ` Bastien
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAC=rjb7G2noq9vm_-LVRN-BLu9uO2+So4A0Np_n6m3i=dfHt=Q@mail.gmail.com' \
--to=ianxm@jhu.edu \
--cc=emacs-orgmode@gnu.org \
--cc=kyle@kyleam.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).