emacs-orgmode
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [O] [PATCH] ob-sql.el: Support sqlcmd and cygwin environment


From: Xi Shen
Subject: Re: [O] [PATCH] ob-sql.el: Support sqlcmd and cygwin environment
Date: Wed, 15 Jun 2016 04:01:06 +0000

Hi Nicolas,

I suppose I should put the news entry to ./etc/ORG-NEWS file, but into which version? I created below entry, please take look and let me know where do you want me to put it.

*** Improved support to Microsoft SQL Server in =ob-sql.el=
=ob-sql.el= library removes support to the ~msosql~ engine which uses
the deprecated =osql= command line tool, and replaces it with ~mssql~
engine which uses the =sqlcmd= command line tool.  Use with properties
like this:

#+BEGIN_EXAMPLE
 :engine mssql
 :dbhost <host.com>
 :dbuser <username>
 :dbpassword <secret>
 :database <database>
#+END_EXAMPLE

If you want to use the *trusted connection* feature, omit *both* the
=dbuser= and =dbpassword= properties and add =cmdline -E= to the
properties.

If your Emacs is running in a Cygwin environment, the =ob-sql.el=
library can pass the converted path to the =sqlcmd= tool.

I checked the code and it does not quote the arguments for me. It is a safe manner in Windows to always quote the path. So I will keep it.

I have a question. Currently the table generated by mssql engine has the "affected rows" append to the end, like this.

  |          memberid | username | xx   | flags |
  |-------------------+----------+------+-------|
  |                 1 | GPL      | Indo | NULL  |
  |                 2 | GPL      | Indo | NULL  |
  |                   |          |      |       |
  | (2 rows affected) |          |      |       |

I personally prefer to remove it. Do you or the org community has a preference about this? Maybe I should keep the behavior align with other engines?


On Tue, Jun 14, 2016 at 9:02 PM Xi Shen <address@hidden> wrote:
Hi Nicholas,

Sure, I will add the news and declaration. 

The "format" is required to quote the filename, so the spaces won't surprise sqlcmd. Maybe the arguments will be quoted when passing to the command?


On Tue, Jun 14, 2016, 19:52 Nicolas Goaziou <address@hidden> wrote:
Hello,

Xi Shen <address@hidden> writes:

> I think I uploaded the wrong patch. Sorry~ Please check this one.

It looks good. Thank you. I have one minor comment about it, see below.

Also, could you provide an entry for ORG-NEWS since this is a user
visible change?

> +(defun org-babel-sql-convert-standard-filename (file)
> +  "Convert the file name to OS standard.
> +In Cygwin environment, is uses Cygwin specific function to
> +convert the file name and double quote it. Otherwise, uses Emacs
> +standard conversion function."
> +  (if (fboundp 'cygwin-convert-file-name-to-windows)
> +      (format "\"%s\"" (cygwin-convert-file-name-to-windows file))
> +    (convert-standard-filename file)))

`cygwin-convert-file-name-to-windows' is going to generate a compilation
warning. You need to declare it with `declare-function' at the top of
the file.

I'm also surprised that the function above doesn't return a string
already. Are you sure the `format' part is needed?

Regards,

--
Nicolas Goaziou
--


Thanks,
David S.

--


Thanks,
David S.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]