From: Jack Kamm <firstname.lastname@example.org> To: Jeremie Juste <email@example.com>, Org Mode <firstname.lastname@example.org> Cc: "Berry, Charles" <email@example.com> Subject: Re: [PATCH] async process in R Date: Sat, 02 Oct 2021 15:54:17 -0700 [thread overview] Message-ID: <firstname.lastname@example.org> (raw) In-Reply-To: <87zgrzclb5.fsf@debian-BULLSEYE-live-builder-AMD64> Hi Jeremie, Many thanks for bringing this over the finish line! I'm very glad it made it into Org 9.5. All the tests passed on my end, and and I successfully ran a few async R blocks without any issues. I do have some suggestions for code style below. They apply to both the original patch, as well as followup fixes that have been committed since. > +(defconst ob-session-async-R-indicator "'ob_comint_async_R_%s_%s'") > + > +(defun ob-session-async-org-babel-R-evaluate-session For consistency with the rest of ob-R.el, as well as with the async functions in ob-python, I suggest using a prefix of "org-babel-R-async-". So "ob-session-async-org-babel-R-evaluate-session" would become "org-babel-R-async-evaluate-session", etc. > +(ert-deftest ob-session-async-R-simple-session-async-value () Again, for consistency I suggest renaming the test functions so they are prefixed with "test-ob-R/". For example, "ob-session-async-R-simple-session-async-value" could become "test-ob-R/async-session-simple-value". This would also allow easily running all the R tests using "BTEST_RE='.*ob-R.*' make test". > + (setq user-inject-src-param ess-inject-source) > (setq ess-eval-visibly nil) > + (setq ess-inject-source 'function-and-buffer) > (ess-eval-buffer nil)) > - (setq ess-eval-visibly tmp) > + (setq ess-eval-visibly tmp) > + (setq ess-inject-source user-inject-src-param) Rather than using setq, it would be better to let bind these like so: (let ((ess-eval-visibly nil) (ess-inject-source 'function-and-buffer)) ...code in here... ) This temporarily sets the variables within the let-block, then resets them to their original values afterwards. Then you can also remove the temporary variables "user-inject-src-param" and "ess-eval-visibly-tmp" as well as their defvars at the top of the file. Actually, since the code is already in a let-block, you can simply add these variables to the existing let-statement. > + (list (format org-babel-R-write-object-command > + (if row-names-p "TRUE" "FALSE") > + (if column-names-p > + (if row-names-p "NA" "TRUE") > + "FALSE") > + ".Last.value" > + (org-babel-process-file-name tmp-file 'noquote)) Some parts of ob-session-async-org-babel-R-evaluate-session, such as the above, are duplicated from org-babel-R-evaluate-session; it would be good to reduce duplication by abstracting these out to separate functions. Thanks again for porting this, and for taking care of ob-R.el in general. All the best, Jack
next prev parent reply other threads:[~2021-10-02 22:54 UTC|newest] Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-26 17:13 Jeremie Juste 2021-09-26 18:33 ` Greg Minshall 2021-09-26 19:52 ` Jeremie Juste 2021-09-27 4:04 ` Greg Minshall 2021-09-27 6:48 ` Bastien 2021-09-27 19:21 ` Jeremie Juste 2021-09-28 3:02 ` Jack Kamm 2021-09-27 8:18 ` Bastien 2021-09-27 18:28 ` Berry, Charles via General discussions about Org-mode. 2021-09-27 19:25 ` Jeremie Juste 2021-09-27 20:28 ` Jeremie Juste 2021-09-27 22:56 ` Berry, Charles via General discussions about Org-mode. 2021-09-27 23:40 ` Berry, Charles via General discussions about Org-mode. 2021-09-28 7:34 ` Jeremie Juste 2021-09-28 18:22 ` Berry, Charles via General discussions about Org-mode. 2021-09-28 20:40 ` Jeremie Juste 2021-10-02 22:57 ` Jack Kamm 2021-10-02 22:54 ` Jack Kamm [this message] 2021-10-02 23:14 ` Jack Kamm
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 \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH] async process in R' \ /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).