Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let user control fallback to original babel sql exection function #74

Merged
merged 1 commit into from
Feb 9, 2019
Merged

Let user control fallback to original babel sql exection function #74

merged 1 commit into from
Feb 9, 2019

Conversation

stardiviner
Copy link
Contributor

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

Verified

This commit was signed with the committer’s verified signature.
stardiviner stardiviner
@coveralls
Copy link

Coverage Status

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

@kostafey
Copy link
Owner

kostafey commented Feb 9, 2019

@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
Copy link
Contributor Author

stardiviner commented Feb 10, 2019

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
Copy link
Contributor Author

stardiviner commented Feb 10, 2019

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
Copy link
Owner

kostafey commented Feb 10, 2019

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
Copy link
Contributor Author

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants