From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xi Shen Subject: Re: [PATCH] ob-sql.el: Support sqlcmd and cygwin environment Date: Tue, 14 Jun 2016 13:02:38 +0000 Message-ID: References: <87r3c4em1r.fsf@saiph.selenimh> <87a8isdso8.fsf@saiph.selenimh> <87vb1c0yyn.fsf@saiph.selenimh> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=001a113ad4c229083805353c9f31 Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:47499) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCnzc-0002Ro-3q for Emacs-orgmode@gnu.org; Tue, 14 Jun 2016 09:02:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCnzV-0005sH-D0 for Emacs-orgmode@gnu.org; Tue, 14 Jun 2016 09:02:55 -0400 Received: from mail-oi0-x232.google.com ([2607:f8b0:4003:c06::232]:34701) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCnzV-0005s1-6j for Emacs-orgmode@gnu.org; Tue, 14 Jun 2016 09:02:49 -0400 Received: by mail-oi0-x232.google.com with SMTP id d132so151033908oig.1 for ; Tue, 14 Jun 2016 06:02:48 -0700 (PDT) In-Reply-To: <87vb1c0yyn.fsf@saiph.selenimh> List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: "Emacs-orgmode" To: "Emacs-orgmode@gnu.org" --001a113ad4c229083805353c9f31 Content-Type: text/plain; charset=UTF-8 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 wrote: > Hello, > > Xi Shen 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. --001a113ad4c229083805353c9f31 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Nicholas,

Sure, I will add the news and declaration.=C2=A0

<= p dir=3D"ltr">The "format" is required to quote the filename, so = the spaces won't surprise sqlcmd. Maybe the arguments will be quoted wh= en passing to the command?


On Tue, Jun 14, 2016= , 19:52 Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
Hello,

Xi Shen <davi= dshen84@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)
> +=C2=A0 "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."
> +=C2=A0 (if (fboundp 'cygwin-convert-file-name-to-windows)
> +=C2=A0 =C2=A0 =C2=A0 (format "\"%s\"" (cygwin-con= vert-file-name-to-windows file))
> +=C2=A0 =C2=A0 (convert-standard-filename file)))

`cygwin-convert-file-name-to-windows' is going to generate a compilatio= n
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<= br> already. Are you sure the `format' part is needed?

Regards,

--
Nicolas Goaziou
--


Thanks,
David S.

--001a113ad4c229083805353c9f31--