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 gKzCKU9flF89RQAA0tVLHw (envelope-from ) for ; Sat, 24 Oct 2020 17:07:27 +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 2GWXJU9flF9kVQAAB5/wlQ (envelope-from ) for ; Sat, 24 Oct 2020 17:07:27 +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 95DD1940111 for ; Sat, 24 Oct 2020 17:07:26 +0000 (UTC) Received: from localhost ([::1]:38648 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kWN0n-000574-Ho for larch@yhetil.org; Sat, 24 Oct 2020 13:07:25 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:33304) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kWMyt-0003SI-Ue for emacs-orgmode@gnu.org; Sat, 24 Oct 2020 13:05:27 -0400 Received: from pb-smtp2.pobox.com ([64.147.108.71]:60535) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kWMyp-00060u-Pf for emacs-orgmode@gnu.org; Sat, 24 Oct 2020 13:05:27 -0400 Received: from pb-smtp2.pobox.com (unknown [127.0.0.1]) by pb-smtp2.pobox.com (Postfix) with ESMTP id 4D37B7BFD2; Sat, 24 Oct 2020 13:05:21 -0400 (EDT) (envelope-from kyle@kyleam.com) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=pobox.com; h=from:to:cc :subject:in-reply-to:references:date:message-id:mime-version :content-type; s=sasl; bh=+2dAD+HBxaxXdToc15zBq6/Bpig=; b=YekH9q 0WJ/doB18FHZ3Uo8OgkhvdNhY3N8cRY4XMfZ6TK5UZa1V36cBAk8GJuyz8osbE16 nhnS0CiItj3Gw3iToHrdhTXYZgafFZl7Y/JxlNNyd7yFQW/uBHUOj82Kh1oe1F84 Jt4kAan8KSOe71Vc4E9uPAf2Vq19VrmvRWuO8= Received: from pb-smtp2.nyi.icgroup.com (unknown [127.0.0.1]) by pb-smtp2.pobox.com (Postfix) with ESMTP id 4164D7BFD1; Sat, 24 Oct 2020 13:05:21 -0400 (EDT) (envelope-from kyle@kyleam.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed; d=kyleam.com; h=from:to:cc:subject:in-reply-to:references:date:message-id:mime-version:content-type; s=mesmtp; bh=uPUz8vID4heEXyIzGPPkxmez6TzAuTbI/IoaNAvOXW8=; b=tZaQ7NArxYAP5zZvUZRsJPfrbprUHBu6DWO/TwBFjUGSJqxydZp89luSasUbDXe5xS+jxXaaLmlJxLQWL6T7y6ePipl7532bKIpM7L/NPNe0jZJu+ZZbmcczZ2CibdRuHOiQqjF1/1uKEuf0FDK2DicORBcbNzXw+5jWsyuBvj8= Received: from localhost (unknown [45.33.91.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by pb-smtp2.pobox.com (Postfix) with ESMTPSA id A87087BFD0; Sat, 24 Oct 2020 13:05:20 -0400 (EDT) (envelope-from kyle@kyleam.com) From: Kyle Meyer To: ian martins Subject: Re: [PATCH] ob-java In-Reply-To: References: Date: Sat, 24 Oct 2020 13:05:19 -0400 Message-ID: <87y2jv367k.fsf@kyleam.com> MIME-Version: 1.0 Content-Type: text/plain X-Pobox-Relay-ID: 192A968A-161B-11EB-8F60-74DE23BA3BAF-24757444!pb-smtp2.pobox.com Received-SPF: pass client-ip=64.147.108.71; envelope-from=kyle@kyleam.com; helo=pb-smtp2.pobox.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/10/24 13:05:21 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] [fuzzy] X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham 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=pass header.d=pobox.com header.s=sasl header.b=YekH9q 0; dkim=pass header.d=kyleam.com header.s=mesmtp header.b=tZaQ7NAr; dmarc=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: -1.21 X-TUID: E1Of5geGalyy 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.