From: ian martins <email@example.com> To: Kyle Meyer <firstname.lastname@example.org> Cc: Org-Mode mailing list <email@example.com> Subject: Re: [PATCH] ob-java Date: Sat, 24 Oct 2020 22:10:12 -0400 [thread overview] Message-ID: <CAC=rjb7qa1NCVj=mbHjAkzXZPK=uCnqkpuQVe_NApAOdfirstname.lastname@example.org> (raw) In-Reply-To: <email@example.com> [-- Attachment #1: Type: text/plain, Size: 5834 bytes --] 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 <firstname.lastname@example.org> 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. > [-- Attachment #2: Type: text/html, Size: 7387 bytes --]
next prev parent reply other threads:[~2020-10-25 2:11 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 2020-10-25 2:10 ` ian martins [this message] 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://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=rjb7qa1NCVj=mbHjAkzXZPK=uCnqkpuQVe_NApAOdemail@example.com' \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH] ob-java' \ /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
Code repositories for project(s) associated with this 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).