From: Kyle Meyer <firstname.lastname@example.org> To: ian martins <email@example.com> Cc: Org-Mode mailing list <firstname.lastname@example.org> Subject: Re: [PATCH] ob-java Date: Sat, 24 Oct 2020 13:05:19 -0400 Message-ID: <email@example.com> (raw) In-Reply-To: <CAC=rjb5ed68AK0umWwjphDoc2ETdcp=kqptEXun=nV+STUC9Qg@mail.gmail.com> 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.
next prev parent reply other threads:[~2020-10-24 17:07 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-05 12:35 ian martins 2020-10-05 13:23 ` ian martins 2020-10-09 11:15 ` ian martins 2020-10-20 18:28 ` John Herrlin 2020-10-20 19:17 ` John Herrlin 2020-10-21 2:37 ` ian martins 2020-10-21 5:59 ` John Herrlin 2020-10-21 12:47 ` ian martins 2020-10-21 13:54 ` John Herrlin 2020-10-22 12:23 ` ian martins 2020-10-22 12:56 ` John Herrlin 2020-10-24 17:05 ` Kyle Meyer [this message] 2020-10-25 2:10 ` ian martins 2020-10-25 2:40 ` Kyle Meyer 2020-10-25 19:36 ` ian martins 2020-11-05 16:29 ` Jarmo Hurri 2020-11-05 17:10 ` ian martins 2020-11-06 5:21 ` Jarmo Hurri 2020-11-06 23:00 ` ian martins 2020-11-09 14:06 ` Jarmo Hurri 2020-11-10 13:14 ` ian martins 2020-11-10 6:29 ` Jarmo Hurri 2020-11-14 11:47 ` Jarmo Hurri 2020-11-14 15:46 ` ian martins 2020-11-15 4:36 ` Jarmo Hurri 2020-11-17 12:07 ` ian martins 2020-12-14 5:55 ` Bastien 2020-11-11 7:45 ` Bastien 2020-10-24 11:58 ` Bastien 2020-10-25 0:30 ` ian martins 2020-10-28 9:13 ` Bastien 2020-10-31 11:03 ` ian martins
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://orgmode.org * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.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
Org-mode mailing list This inbox may be cloned and mirrored by anyone: git clone --mirror https://orgmode.org/list/0 list/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 list list/ https://orgmode.org/list \ firstname.lastname@example.org public-inbox-index list Example config snippet for mirrors. Newsgroups are available over NNTP: nntp://news.yhetil.org/yhetil.emacs.orgmode nntp://news.gmane.io/gmane.emacs.orgmode AGPL code for this site: git clone https://public-inbox.org/public-inbox.git