I modified most part of my files. - There are some places I can't improve because I'm not good at elisp. (maybe other people can improve it later) Like ob-redis.el implement ob-sql style configuration. and ob-lua.el make use of lua-mode's running process. - I guess ob-php.el is the only one branch can be merged. - Anyway, check out my updates, if no one can be merged, I will still keep my github repositories. Thanks for your detail reviews. [stardiviner] GPG key ID: 47C32433 IRC(freeenode): stardiviner Twitter: @numbchild Key fingerprint = 9BAA 92BC CDDD B9EF 3B36 CB99 B8C4 B8E5 47C3 2433 Blog: http://stardiviner.github.io/ On Mon, May 23, 2016 at 1:20 AM, Rasmus wrote: > Hi, > > Thanks for your patches. Sorry about the delay. I was hoping that > someone with more ob knowledge would step in. > > Please find some comments below. I think some more work is needed before > your libraries are > > "numbchild@gmail.com" writes: > > > From 2589d4e7d28016fb515d2131cbd9ff52797e50eb Mon Sep 17 00:00:00 2001 > > From: stardiviner > > Date: Tue, 10 May 2016 16:03:32 +0800 > > Subject: [PATCH] ob-lua.el: add Lua src block executing support > > > > * contrib/lisp/ob-lua.el (org-babel-execute:lua): support executing Lua > > src block. > > Please capitalize after the colon. > > > --- > > +;;; ob-lua.el --- Execute Lua code within org-mode blocks. > > +;; Copyright 2016 stardiviner > > > +;; Author: stardiviner > > +;; Maintainer: stardiviner > > > > > +;; Keywords: org babel lua > > For whatever reason this seems to be > > ;; Keywords: literate programming, reproducible research > > In most ob files. > > > +;; URL: https://github.com/stardiviner/ob-lua > > > > +;; Created: 12th April 2016 > > This header is unnecessary, but if you like it, it’s fine. > > > +;; Version: 0.0.1 > > +;; Package-Requires: ((org "8")) > > > +;;; Commentary: > > +;; > > +;; Execute Lua code within org-mode blocks. > > > Maybe, > > The file provides Org-Babel support for evaluating Lua code. > > > > > +;;; Code: > > +(require 'org) > > This is unnecessary. Ob implies Org. > > > +(require 'ob) > > + > > +(defgroup ob-lua nil > > + "org-mode blocks for Lua." > > + :group 'org) > > It seems that ob languages do not typically define new groups. > > Also, ob is the filename. Variables are org-babel. > > > +(defcustom ob-lua:default-session "*lua*" > > + "Default Lua session. > > + > > +It is lua inferior process from `run-lua'." > > + :group 'ob-lua > > + :type 'string) > > I don’t think this is necessary to have as a defcustom. There’s already > :session. Also, you are missing :type. Per above, group is org-babel. > > > +;;;###autoload > This is normally not autoloaded. Babel languages are loaded via > org-babel-do-load-languages. > > > +(defun org-babel-execute:lua (body params) > > + "org-babel lua hook." > > Please capitalize sentences. > > > + (let* ((session (or (cdr (assoc :session params)) > > + ob-lua:default-session)) > > + (cmd (mapconcat 'identity (list "lua -") " "))) > > Is something missing here? AFAIK cmd → "lua -" always. > > Also, what if my lua is not in my PATH? I got a felling that you might > make a more robust mode by hooking into lua-mode > > https://immerrr.github.io/lua-mode/ > > > + (org-babel-eval cmd body))) > > How are various return values handled? E.g. will a table be correctly > interpreted as such? (It’s a while since I coded in lua). > > > +;;;###Autoload > > +(eval-after-load "org" > > + '(add-to-list 'org-src-lang-modes '("lua" . lua))) > > > This should be unnecessary as the lua-mode is presumably lua-mode. Also, > I think your code depends on lua-mode in order to be able to edit it. You > need to declare that as a dependency. > > > +(provide 'ob-lua) > > + > > +;;; ob-lua.el ends here > > -- > > 2.8.2 > > > > > > From d2e7202930fcf24e7c90826e69bb768094463a0c Mon Sep 17 00:00:00 2001 > > From: stardiviner > > Date: Tue, 10 May 2016 16:05:38 +0800 > > Subject: [PATCH] ob-php.el: Add PHP src block executing support > > > > * contrib/lisp/ob-php.el (org-babel-execute:php): support executing PHP > > src block. > > Capitalize. > > > --- > > contrib/lisp/ob-php.el | 44 ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > create mode 100644 contrib/lisp/ob-php.el > > > > diff --git a/contrib/lisp/ob-php.el b/contrib/lisp/ob-php.el > > new file mode 100644 > > index 0000000..31960a5 > > --- /dev/null > > +++ b/contrib/lisp/ob-php.el > > @@ -0,0 +1,44 @@ > > +;;; ob-php.el --- Execute PHP within org-mode blocks. > > +;; Copyright 2016 stardiviner > > + > > +;; Author: stardiviner > > +;; Maintainer: stardiviner > > +;; Keywords: org babel php > > +;; URL: https://github.com/stardiviner/ob-php > > +;; Created: 04th May 2016 > > +;; Version: 0.0.1 > > +;; Package-Requires: ((org "8")) > > + > > +;;; Commentary: > > +;; > > +;; Execute PHP within org-mode blocks. > > + > > +;;; Code: > > +(require 'org) > > +(require 'ob) > > + > > +(defgroup ob-php nil > > + "org-mode blocks for PHP." > > + :group 'org) > > > See comments above. > > > +;; Todo > > Remove. > > > +(defcustom ob-php:inf-php-buffer "*php*" > > + "Default PHP inferior buffer." > > + :group 'ob-php > > + :type 'string) > > :Type is missing and this buffer should not be a defcustom. > > > +;;;###Autoload > > Remove. > > > +(defun org-babel-execute:php (body params) > > + "org-babel PHP hook." > > + ;; Todo > > Remove. > > > + (let* ((cmd (mapconcat 'identity (list "php") " -r "))) > > + (org-babel-eval cmd body) > > + )) > > See comments above re docstring, and robustness. > > What sort of return values can I expect from this? I don’t know php, but > I assume it’s mainly "log messages". > > > +;;;###autoload > > +(eval-after-load "org" > > + '(add-to-list 'org-src-lang-modes '("php" . php))) > > Unnecessary. Also, it seems there’s an undeclared decency on some sort of > php mode. > > > + > > +(provide 'ob-php) > > > From eb3c0cb1467416d141a633c49e1c9050311d92ab Mon Sep 17 00:00:00 2001 > > From: stardiviner > > Date: Tue, 10 May 2016 16:06:19 +0800 > > Subject: [PATCH] ob-redis.el: Add Redis src block executing support > > > > * contrib/lisp/ob-redis.el (org-babel-execute:redis): support for > > executing redis src block. > > Capitalize > > > > --- > > contrib/lisp/ob-redis.el | 44 > ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > create mode 100644 contrib/lisp/ob-redis.el > > > > diff --git a/contrib/lisp/ob-redis.el b/contrib/lisp/ob-redis.el > > new file mode 100644 > > index 0000000..340b050 > > --- /dev/null > > +++ b/contrib/lisp/ob-redis.el > > @@ -0,0 +1,44 @@ > > +;;; ob-redis.el --- Execute Redis queries within org-mode blocks. > > +;; Copyright 2016 stardiviner > > + > > +;; Author: stardiviner > > +;; Maintainer: stardiviner > > +;; Keywords: org babel redis > > +;; URL: https://github.com/stardiviner/ob-redis > > +;; Created: 28th Feb 2016 > > +;; Version: 0.0.1 > > +;; Package-Requires: ((org "8")) > > + > > +;;; Commentary: > > +;; > > +;; Execute Redis queries within org-mode blocks. > > + > > +;;; Code: > > +(require 'org) > > +(require 'ob) > > + > > +(defgroup ob-redis nil > > + "org-mode blocks for Redis." > > + :group 'org) > > As above. > > > +(defcustom ob-redis:default-db "127.0.0.1:6379" > > + "Default Redis database." > > + :group 'ob-redis > > + :type 'string) > > Does redis support login and different ports and all that jazz? > If so, please compare the configurability to the needs of ob-sql. > > Is it possible to extend ob-sql with another engine? (I don’t know > anything about redis). > > Or are these "NoSQL" databases similar enough that it would be possible to > have "unite" them in a single library. Does ob have support for any > "NoSQL" databases ATM? > > I’m not implying that this is necessarily the right way to go. I’m just > trying to understand the nature of this. > > > +;;;###Autoload > > Remove. > > > +(defun org-babel-execute:redis (body params) > > + "org-babel redis hook." > > + (let* ((db (or (cdr (assoc :db params)) > > + ob-redis:default-db)) > > + (cmd (mapconcat 'identity (list "redis-cli") " "))) > > + (org-babel-eval cmd body) > > + )) > > > My guess is that connectivity support is not comprehensive enough. > > > +;;;###autoload > > +(eval-after-load "org" > > + '(add-to-list 'org-src-lang-modes '("redis" . redis))) > > Remove. > > > +(provide 'ob-redis) > > + > > +;;; ob-redis.el ends here > > Thanks, > Rasmus > > -- > Vote for proprietary math! > > > >