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.