Let user control fallback to original babel sql exection function

[stardiviner]

User might want to use default sql exection function when mixing SQL code in one Org file. Or in some special cases.

[coveralls]

Coverage increased (+0.4%) to 39.69% when pulling 2cf38ff on stardiviner:fallback-to-default-babel into a73e3c6 on kostafey:master.

[kostafey]

@stardiviner, reverted since it can't be used in the case when org-babel-execute:sql is undefined.
Only existing functions can be adviced. So, it failes with
org-babel-execute-src-block: No org-babel-execute function for sql!
despite ejc-org-mode-babel-wrapper is t.

Furthermore, why do you use (cdr (assq :engine params))? Assume, someone has a case, when engine is defined, but he want to enforce use ejc-sql.

  (if (or (not ejc-org-mode-babel-wrapper)
          (cdr (assq :engine params)))

I guess

  (if (not ejc-org-mode-babel-wrapper)

is enough. If one want to use ejc-sql, he can enabe it, if won't - disable.

Please, consider both cases, when org-babel-execute:sql is defined or not, and make a new
pull request.

[stardiviner]

Furthermore, why do you use (cdr (assq :engine params))? Assume, someone has a case, when engine is defined, but he want to enforce use ejc-sql.

About this, I think because user might mix two types sql blocks in one Org buffer, for example, there are some sql blocks want need to specify :engine to execute without ejc-sql. Some blocks need be executed by ejc-sql because ejc-sql execution does not need :engine header argument anyway, but Org sql blocks use it to differ sql databases. One words, :engine is unnecessary for ejc-sql. I know your said case, user usually already know which block use which eval function to execute. If want to take care of all situation, I think might add an ask prompt for user like this presudo code flow:

(if (sql-block)
  (if (and (has :engine header)
           (yes-or-no-p ask user whether to use ejc-sql still?))
      (for yes, then use ejc-sql)
      (for no, use original babel sql exection function))))

WDYT?

I hope ejc-sql and Org original execute function can exist in one buttfer together.

in the case when org-babel-execute:sql is undefined.

This case, I think (require 'ob-sql) explicitly function code before advice-add can solved.

If you think this is ok, any suggestion welcome. I will make a new PR.

[stardiviner]

Here is the updated logic:

@@ -341,8 +341,9 @@ Prepare buffer to operate as `ejc-sql-mode' buffer."
 
 (defun ejc-eval-org-snippet (&optional orig-fun body params)
   "Used to eval SQL code in `org-mode' code snippets."
-  (if (or (not ejc-org-mode-babel-wrapper)
-          (cdr (assq :engine params)))
+  (if (and (not ejc-org-mode-babel-wrapper)
+           (cdr (assq :engine params))
+           (yes-or-no-p "ejc-sql is enabled, use it to execute sql block? "))
       (funcall orig-fun body params)
     (let* ((beg (save-excursion
                   (goto-char (nth 5 (org-babel-get-src-block-info)))

WDYT?

[kostafey]

This case, I think (require 'ob-sql) explicitly function code before advice-add can solved.

Yes, it's perfectly ok.

About choosing execution engine logic:

1. ejc-org-mode-babel-wrapper is nil
   ok nothing to do with ejc-sql here (it's totally disabled), use orig-fun

2. ejc-org-mode-babel-wrapper is t and no :engine parameter
   ok, no any uncertainty here - use ejc-sql

3. ejc-org-mode-babel-wrapper is t and :engine parameter is defined
   ok, we have an uncertainty here - so ask user what he wants to do

After all, I think this matches logic described above:

  (if (or (not ejc-org-mode-babel-wrapper)
          (and (cdr (assq :engine params))
               (not
                (yes-or-no-p
                 (concat "ejc-sql is enabled, ignore block connection"
                         " params and use ejc-sql to execute it? ")))))
      (funcall orig-fun body params)
    (let* ((beg (save-excursion
      ...

Also, please make a note about ejc-org-mode-babel-wrapper in README and create new pull request.

[stardiviner]

Ok, I updated, also mentioned this choosing engine logic in README, also add a link to this discussion.

I checked out master branch, this PR status is previous merged, then revered, so I just force-pushed commit again, so might no need to create a new PR.

[kostafey]

Unfortunately, I already merged and reversed it, so looks like a new pull request is required.