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

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Xi Shen <davidshen84@gmail.com> 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