add quoteShellCommand

[timotheecour]

same as:

• in D: escapeShellCommand
• in C#: QuoteArguments
• in perl: String::ShellQuote
• in node: quote
• in ruby: shelljoin
• in go: shellquote

[dom96]

3 (and now 4 with this PR) quoting procedures seems like going a bit overboard. Why do we have so many?

[timotheecour]

quoteShellCommand (Nim's version of https://dlang.org/library/std/process/escape_shell_command.html) quotes command lines (ie array of arguments), whereas quoteShell (Nim's version of https://dlang.org/library/std/process/escape_shell_file_name.html) quotes only a single argument.

If anything, quoteShellWindows and quoteShellPosix could be merged into the existing quoteShell, but that's a separate discussion (and there are arguments for allowing cross platform quoting, eg as a linux web service that serves windows users, etc).

[andreaferretti]

I don't know a nice way to say that, but if you want to copy everything from D, maybe you should just use D.

(That said, I have nothing against this particular PR)

[dom96]

quoteShellCommand (Nim's version of https://dlang.org/library/std/process/escape_shell_command.html) quotes command lines (ie array of arguments), whereas quoteShell (Nim's version of https://dlang.org/library/std/process/escape_shell_file_name.html) quotes only a single argument.

So wouldn't it be better to overload quoteShellCommand quoteShell instead of coming up with another name?

If anything, quoteShellWindows and quoteShellPosix could be merged into the existing quoteShell, but that's a separate discussion (and there are arguments for allowing cross platform quoting, eg as a linux web service that serves windows users, etc).

Yes, I think that should be done. That way we can just have quoteShellCommand and nothing else. But perhaps there is a good reason these are separate.

[dom96]

I don't know a nice way to say that, but if you want to copy everything from D, maybe you should just use D.

I can understand this sentiment. But when using a new language it's only natural that you look for functions in the stdlib which are available in the language you are most familiar with. If these functions are simply not available then it makes sense to add them, especially if these functions are also available in other languages (which I am guessing they are).

[cdunn2001]

First, quoteShellCommand() should be called quoteShellCmd(), to match os.execShellCmd().

Second, this procedure should accept the "quote function" as a parameter, with quoteShellPosix as the default.

But it's a one-liner. So I'm with @dom96 on this.

I'd favor adding the one-liner to the docs for quoteShell(). Sometimes, docs are better than extra API. We don't spend enough time on Nim docs, and I admit that I was not familiar with mapIt() versus map() myself.

[timotheecour]

@cdunn2001 @dom96
turns out this simple 1 liner args.map(quoteShell).join(" ") is not as simple to do as it seems, I'm hitting against type inference limitations: see #8325 (comment)

[cdunn2001]

The restriction of map against cdecl is a bit annoying. Did you try mapIt()? What about the deprecated mapIt() that takes a type?

• https://nim-lang.org/docs/sequtils.html#mapIt.t,untyped,untyped,untyped

[timotheecour]

@cdunn2001

The restriction of map against cdecl is a bit annoying. Did you try mapIt()? What about the deprecated mapIt() that takes a type?

unfortunately this also doesn't work because openArray can't be used with let, eg:

import sequtils
import strutils

proc fun(a:string):auto{.cdecl.}=a

when true:
 # not ok with cdecl: Error: type mismatch: got <openarray[string]...
 proc test(args:openArray[string])=
  echo args.map(fun).join(" ")

elif true:
 # Error: invalid type: 'openarray[string]' for let
 # ok cdecl but not ok with openArray
 proc test(args:openArray[string])=
  echo args.mapIt(fun(it)).join(" ")
else:
 # ok with cdecl but won't accept same stuff as openArray
 proc test(args:seq[string])=
  echo args.mapIt(fun(it)).join(" ")

let args = @["foo"]
test(args)

[Varriount]

@timotheecour Tests have failed.

[timotheecour]

I tried to revive this now that #8543 was just merged fresh from the oven (which was blocking this); however, it's running into a fresh new bootstrapping issue #8577

[timotheecour]

PTAL : tests are green now that I fixed #8577

[Araq]

Why do we need it? It's confusing. Don't add it. Your explanation was in D-specific terms that I can't follow.

[timotheecour]

Why do we need it? It's confusing. Don't add it. Your explanation was in D-specific terms that I can't follow.

the exact same functionality exists in D, C#, perl, node, ruby, go, and probably much more (I added links to all these functions in top-level message).

It's used for example to safely escape an array of arguments to a function accepting a string, eg execCmdEx

even though some procs in osproc.nim accept an array of arguments, they're not easily interchangeable and in some cases using a string is more convenient (maybe the string will be constructed partially from an existing shell quoted string, and appended to quoteShellCommand obtained from other arguments). Since proper quoting is a common source of bugs (see list of issues in github), making it a function in stdlib instead of letting user inline the code each time makes sense.

[Araq]

Ok, then write it without the sequtils dependency. It will also avoid a temporay sequence creation this way then, maybe, too tired to think about it.

[timotheecour]

Ok, then write it without the sequtils dependency. It will also avoid a temporay sequence creation this way then, maybe, too tired to think about it.

done, used a for-loop; indeed a temp sequence would be created via mapIt because mapIt is not lazy. Hopefully sequtils can eventually be deprecated for lazy versions of all these, eg smthg similar to zero_functional or other. But that's a topic for another PR.

PTAL

[dom96]

Can we remove/merge the other quoting procs while we're at it?

[Araq]

No, these are rather well designed.