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

[Xi Shen]

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.