From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp2 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id YOX2J8DelF89IgAA0tVLHw (envelope-from ) for ; Sun, 25 Oct 2020 02:11:12 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp2 with LMTPS id SLDZI8DelF+vNgAAB5/wlQ (envelope-from ) for ; Sun, 25 Oct 2020 02:11:12 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 11DEA940111 for ; Sun, 25 Oct 2020 02:11:10 +0000 (UTC) Received: from localhost ([::1]:45122 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kWVUy-0000f9-3B for larch@yhetil.org; Sat, 24 Oct 2020 22:11:08 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:46494) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kWVUL-0000ce-Ed for emacs-orgmode@gnu.org; Sat, 24 Oct 2020 22:10:29 -0400 Received: from mail-ed1-f54.google.com ([209.85.208.54]:41960) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kWVUH-00050O-Nk for emacs-orgmode@gnu.org; Sat, 24 Oct 2020 22:10:29 -0400 Received: by mail-ed1-f54.google.com with SMTP id l24so5762336edj.8 for ; Sat, 24 Oct 2020 19:10:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=KNQ1r3uK2QtRoPXFqZI6q5rGcnK7lJC0oo5DqlJIiIk=; b=pIOMMVFt/TwxH5hncW7/YHqP63lh3LVKtH/+sgj4bfsnM51iTiHah5EP2SCwgLn8U/ VgHu83xExSFNudUQKlyrRhGbYLa30l1oB7JQ9UPPXZiMQR7+BTHhCYBOmQtsaRxzniV0 Yuqi/xrgCeiF5mLorM95V2w3Pk8FoHJYEOb15ACogYmNpOm4VfepqO85zTPDFPHvJktd DVhPDp+1Ige9NCcIO7WUfWn6py7gkBZgraDIQGuYvlwitU7zHBRvMh2F3eTo7ZsZVFNi 6L+QKNa8CHD7EZ8/hDzs7KI6FcsGHdOSZr9l7OcbQD3CchgKB/9EogaAfpAHooQs76xU 1Uqg== X-Gm-Message-State: AOAM531MyLBJtJMqJGztMFlN3xW9qBx0j9+pLxZNrOzVU/wQ4sqOX0f2 sVMbLKdPatAvV83jhloHlH05Ja64BJS8JiJS9Mpl0HLRiOg= X-Google-Smtp-Source: ABdhPJzqqEergoQZiG5NhuE/yER9xbn+N6lB3V0ZlYeTEks1CrxGJ6e7cgSNV6Zl8Wt3g3d0ujEcd8pLrcDxonT1C+g= X-Received: by 2002:a05:6402:1686:: with SMTP id a6mr8323956edv.106.1603591823829; Sat, 24 Oct 2020 19:10:23 -0700 (PDT) MIME-Version: 1.0 References: <87y2jv367k.fsf@kyleam.com> In-Reply-To: <87y2jv367k.fsf@kyleam.com> From: ian martins Date: Sat, 24 Oct 2020 22:10:12 -0400 Message-ID: Subject: Re: [PATCH] ob-java To: Kyle Meyer Content-Type: multipart/alternative; boundary="000000000000062f9605b27551ec" Received-SPF: pass client-ip=209.85.208.54; envelope-from=ianxm1@gmail.com; helo=mail-ed1-f54.google.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/10/24 22:10:24 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] [fuzzy] X-Spam_score_int: -10 X-Spam_score: -1.1 X-Spam_bar: - X-Spam_report: (-1.1 / 5.0 requ) BAYES_00=-1.9, FREEMAIL_ENVFROM_END_DIGIT=0.25, FREEMAIL_FORGED_FROMDOMAIN=0.25, FREEMAIL_FROM=0.001, HEADER_FROM_DIFFERENT_DOMAINS=0.25, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=no autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-orgmode@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Org-Mode mailing list Errors-To: emacs-orgmode-bounces+larch=yhetil.org@gnu.org Sender: "Emacs-orgmode" X-Scanner: scn0 Authentication-Results: aspmx1.migadu.com; dkim=none; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=jhu.edu (policy=none); spf=pass (aspmx1.migadu.com: domain of emacs-orgmode-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=emacs-orgmode-bounces@gnu.org X-Spam-Score: -0.91 X-TUID: RxAc6gpprOKa --000000000000062f9605b27551ec Content-Type: text/plain; charset="UTF-8" Thanks for the feedback. In general, the changes are all intended to be backwards compatible. Thanks for pointing out changes that weren't. After making the changes, should I submit updated patches or is it fine to push? On Sat, Oct 24, 2020 at 1:05 PM Kyle Meyer wrote: > Hi ian, > > ian martins writes: > > >> * Changes > >> > >> - support for functional mode (~:results value~) > >> - accept variables > >> - don't require package, class, and main definitions > >> - write source and result tempfiles to ~org-babel-temporary-directory~, > >> but respects the ~:dir~ header > >> - work with tramp > > Thanks for all the work you've put into this. As someone that knows > nothing about Java and hasn't touched ob-java, that sounds good :) > > I see this got a "feel free to install" elsewhere in this thread, so > here are a few scattered and superficial remarks. > > > Subject: [PATCH] ob-java.el: Add support for variables, return values, > tramp > > > > * lisp/ob-java.el: Add support for variables and return values. Write > > tempfiles to the org-babel-temporary-directory. Make package, class, > > and main method definitions optional. > > > > * testing/lisp/test-ob-java.el: Add tests. > > I think the changelog entry should technically have > per-function/variable entries. > > More importantly from my perspective, it would be great for the message > to explain what the current state is, why it is problematic, and what > the patch's solution is. For this patch, that'd probably be much too > large, which is a good indication that it'd be better presented as a > split up series. But, at this point, that's not something I think you > should do for this patch, especially given there doesn't seem to be a > java/ob-java user with the bandwidth to provide a detailed review. > > > -(defcustom org-babel-java-command "java" > > - "Name of the java command. > > -May be either a command in the path, like java > > -or an absolute path name, like /usr/local/bin/java > > -parameters may be used, like java -verbose" > > +(defvar org-babel-default-header-args:java '() > > + "Default header args for java source blocks.") > > + > > +(defconst org-babel-header-args:java '((imports . :any)) > > + "Java-specific header arguments.") > > + > > +(defvar org-babel-java-compiler-command "javac" > > + "Name of the command to execute the java compiler.") > > + > > +(defvar org-babel-java-runtime-command "java" > > + "Name of the command to run the java runtime.") > > Rather than dropping org-babel-java-command and org-babel-java-compiler > entirely, can you make this change in a way that doesn't break for users > that have already customized org-babel-java-command? > > Also, shouldn't org-babel-java-compiler-command and > org-babel-java-runtime-command be user options rather than defvars? > > In general, are the changes here expected to be backwards compatible for > current ob-java users? > > > +(defcustom org-babel-java-hline-to "null" > > + "Replace hlines in incoming tables with this when translating to > java." > > :group 'org-babel > > - :version "24.3" > > + :version "25.2" > > + :package-version '(Org . "9.3") > > :type 'string) > > > > -(defcustom org-babel-java-compiler "javac" > > - "Name of the java compiler. > > -May be either a command in the path, like javac > > -or an absolute path name, like /usr/local/bin/javac > > -parameters may be used, like javac -verbose" > > +(defcustom org-babel-java-null-to 'hline > > + "Replace `null' in java tables with this before returning." > > :group 'org-babel > > - :version "24.3" > > - :type 'string) > > + :version "25.2" > > + :package-version '(Org . "9.3") > > + :type 'symbol) > > For both these options, s/9.3/9.5/. And please drop :version, letting > it be handled by customize-package-emacs-version-alist. > > > (defun org-babel-execute:java (body params) > > - (let* ((classname (or (cdr (assq :classname params)) > > - (error > > - "Can't compile a java block without a > classname"))) > > - (packagename (file-name-directory classname)) > > - (src-file (concat classname ".java")) > > + "Execute a java source block with BODY code and PARAMS params." > > + (let* (;; if true, run from babel temp directory > > + (run-from-temp (not (assq :dir params))) > > + ;; class and package > > + (fullclassname (or (cdr (assq :classname params)) > > + (org-babel-java-find-classname body))) > > + ;; just the class name > > + (classname (car (last (split-string fullclassname "\\.")))) > > + ;; just the package name > > + (packagename (if (seq-contains fullclassname ?.) > > Please avoid seq- for compatibility with older versions of Emacs. > > > +(defun org-babel-java-evaluate (cmd result-type result-params > result-file) > > + "Evaluate using an external java process. > > +CMD the command to execute. > > + > > +If RESULT-TYPE equals 'output then return standard output as a > > +string. If RESULT-TYPE equals 'value then return the value > > +returned by the source block, as elisp. > > + > > +RESULT-PARAMS input params used to format the reponse. > > + > > +RESULT-FILE filename of the tempfile to store the returned value in > > +for 'value RESULT-TYPE. Not used for 'output RESULT-TYPE." > > Convention nit: Prefer `symbol' to 'symbol (e.g., s/'output/`output'/). > I didn't check closely if this applies to other docstrings in this > patch. > > > + (let ((raw (pcase result-type > > + ('output (org-babel-eval cmd "")) > > + ('value (org-babel-eval cmd "") > > + (org-babel-eval-read-file result-file))))) > > 'VAL isn't compatible with all the Emacs versions we support. Please > use `VAL instead. > --000000000000062f9605b27551ec Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks for the feedback. In general, the changes are = all intended to be backwards compatible. Thanks for pointing out changes th= at weren't.

After making the changes, should I= submit updated patches or is it fine to push?

On Sat, Oct 24, 2020 at= 1:05 PM Kyle Meyer <kyle@kyleam.com<= /a>> wrote:
H= i ian,

ian martins writes:

>> * Changes
>>
>> - support for functional mode (~:results value~)
>> - accept variables
>> - don't require package, class, and main definitions
>> - write source and result tempfiles to ~org-babel-temporary-direct= ory~,
>> but respects the ~:dir~ header
>> - work with tramp

Thanks for all the work you've put into this.=C2=A0 As someone that kno= ws
nothing about Java and hasn't touched ob-java, that sounds good :)

I see this got a "feel free to install" elsewhere in this thread,= so
here are a few scattered and superficial remarks.

> Subject: [PATCH] ob-java.el: Add support for variables, return values,= tramp
>
> * lisp/ob-java.el: Add support for variables and return values.=C2=A0 = Write
> tempfiles to the org-babel-temporary-directory.=C2=A0 Make package, cl= ass,
> and main method definitions optional.
>
> * testing/lisp/test-ob-java.el: Add tests.

I think the changelog entry should technically have
per-function/variable entries.

More importantly from my perspective, it would be great for the message
to explain what the current state is, why it is problematic, and what
the patch's solution is.=C2=A0 For this patch, that'd probably be m= uch too
large, which is a good indication that it'd be better presented as a split up series.=C2=A0 But, at this point, that's not something I think= you
should do for this patch, especially given there doesn't seem to be a java/ob-java user with the bandwidth to provide a detailed review.

> -(defcustom org-babel-java-command "java"
> -=C2=A0 "Name of the java command.
> -May be either a command in the path, like java
> -or an absolute path name, like /usr/local/bin/java
> -parameters may be used, like java -verbose"
> +(defvar org-babel-default-header-args:java '()
> +=C2=A0 "Default header args for java source blocks.")
> +
> +(defconst org-babel-header-args:java '((imports . :any))
> +=C2=A0 "Java-specific header arguments.")
> +
> +(defvar org-babel-java-compiler-command "javac"
> +=C2=A0 "Name of the command to execute the java compiler.")=
> +
> +(defvar org-babel-java-runtime-command "java"
> +=C2=A0 "Name of the command to run the java runtime.")

Rather than dropping org-babel-java-command and org-babel-java-compiler
entirely, can you make this change in a way that doesn't break for user= s
that have already customized org-babel-java-command?

Also, shouldn't org-babel-java-compiler-command and
org-babel-java-runtime-command be user options rather than defvars?

In general, are the changes here expected to be backwards compatible for current ob-java users?

> +(defcustom org-babel-java-hline-to "null"
> +=C2=A0 "Replace hlines in incoming tables with this when transla= ting to java."
>=C2=A0 =C2=A0 :group 'org-babel
> -=C2=A0 :version "24.3"
> +=C2=A0 :version "25.2"
> +=C2=A0 :package-version '(Org . "9.3")
>=C2=A0 =C2=A0 :type 'string)
>=C2=A0
> -(defcustom org-babel-java-compiler "javac"
> -=C2=A0 "Name of the java compiler.
> -May be either a command in the path, like javac
> -or an absolute path name, like /usr/local/bin/javac
> -parameters may be used, like javac -verbose"
> +(defcustom org-babel-java-null-to 'hline
> +=C2=A0 "Replace `null' in java tables with this before retur= ning."
>=C2=A0 =C2=A0 :group 'org-babel
> -=C2=A0 :version "24.3"
> -=C2=A0 :type 'string)
> +=C2=A0 :version "25.2"
> +=C2=A0 :package-version '(Org . "9.3")
> +=C2=A0 :type 'symbol)

For both these options, s/9.3/9.5/.=C2=A0 And please drop :version, letting=
it be handled by customize-package-emacs-version-alist.

>=C2=A0 (defun org-babel-execute:java (body params)
> -=C2=A0 (let* ((classname (or (cdr (assq :classname params))
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0(error
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 "Can't compile a java block without a classname"))) > -=C2=A0 =C2=A0 =C2=A0 (packagename (file-name-directory classname)) > -=C2=A0 =C2=A0 =C2=A0 (src-file (concat classname ".java"))<= br> > +=C2=A0 "Execute a java source block with BODY code and PARAMS pa= rams."
> +=C2=A0 (let* (;; if true, run from babel temp directory
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(run-from-temp (not (assq :dir para= ms)))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0;; class and package
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(fullclassname (or (cdr (assq :clas= sname params))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 (org-babel-java-find-classname body)))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0;; just the class name
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(classname (car (last (split-string= fullclassname "\\."))))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0;; just the package name
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(packagename (if (seq-contains full= classname ?.)

Please avoid seq- for compatibility with older versions of Emacs.

> +(defun org-babel-java-evaluate (cmd result-type result-params result-= file)
> +=C2=A0 "Evaluate using an external java process.
> +CMD the command to execute.
> +
> +If RESULT-TYPE equals 'output then return standard output as a > +string.=C2=A0 If RESULT-TYPE equals 'value then return the value<= br> > +returned by the source block, as elisp.
> +
> +RESULT-PARAMS input params used to format the reponse.
> +
> +RESULT-FILE filename of the tempfile to store the returned value in > +for 'value RESULT-TYPE.=C2=A0 Not used for 'output RESULT-TYP= E."

Convention nit: Prefer `symbol' to 'symbol (e.g., s/'output/`ou= tput'/).
I didn't check closely if this applies to other docstrings in this
patch.

> +=C2=A0 (let ((raw (pcase result-type
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0('output (= org-babel-eval cmd ""))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0('value (o= rg-babel-eval cmd "")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0(org-babel-eval-read-file result-file)))))

'VAL isn't compatible with all the Emacs versions we support.=C2=A0= Please
use `VAL instead.
--000000000000062f9605b27551ec--