emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Rasmus <rasmus@gmx.us>
To: emacs-orgmode@gnu.org
Subject: Re: add some babel supports (PHP, Lua, Redis)
Date: Sun, 22 May 2016 19:20:13 +0200	[thread overview]
Message-ID: <87ziri9fhu.fsf@gmx.us> (raw)
In-Reply-To: CAL1eYu+Nq_Bp4VXXTfCd7YN5ab-XQmZC5pUe_D0E6wzw0p1Scw@mail.gmail.com

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" <numbchild@gmail.com> writes:

> From 2589d4e7d28016fb515d2131cbd9ff52797e50eb Mon Sep 17 00:00:00 2001
> From: stardiviner <numbchild@gmail.com>
> 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 <numbchild@gmail.com>
> +;; Maintainer: stardiviner <numbchild@gmail.com>



> +;; 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 <numbchild@gmail.com>
> 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 <numbchild@gmail.com>
> +;; Maintainer: stardiviner <numbchild@gmail.com>
> +;; 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 <numbchild@gmail.com>
> 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 <numbchild@gmail.com>
> +;; Maintainer: stardiviner <numbchild@gmail.com>
> +;; 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!

  parent reply	other threads:[~2016-05-22 17:20 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-05-10  8:07 add some babel supports (PHP, Lua, Redis) numbchild
2016-05-10  9:42 ` Rasmus
2016-05-10 15:23   ` numbchild
2016-05-10 15:44     ` Rasmus
2016-05-10 15:47       ` Ken Mankoff
2016-05-10 15:58         ` Rasmus
2016-05-10 16:12           ` Ken Mankoff
2016-05-11  1:42       ` numbchild
2016-05-22 17:20     ` Rasmus [this message]
2016-05-23  3:09       ` numbchild
2016-05-27 16:52         ` Rasmus
2016-05-28  0:50           ` numbchild

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=87ziri9fhu.fsf@gmx.us \
    --to=rasmus@gmx.us \
    --cc=emacs-orgmode@gnu.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public 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).