resolvePath() can't return more than one path

[jhanssen]

Looks like resolvePath() can't return more than one path, this makes globbing * hard since that will likely result in more than one path. I'm currently working around this by returning a magic value and then doing globbing when I encounter the value in the AST but it seems this should be handled by bash-parser internally. In addition, globbing might involve file system access and as such it would be useful if it could be done asynchronously. Right now the value needs to be returned directly from resolvePath().

Thanks!

[parro-it]

Looks like resolvePath() can't return more than one path, this makes globbing * hard since that will likely result in more than one path.

That's because you are supposed to return the whole token with globbing expanded. E.g. if you receive as argument '/tmp/*' (supposing you resolve the globbing to two files: '/tmp/a' and '/tmp/b') you are supposed to return q single string '/tmp/a /tmp/b'.

In addition, globbing might involve file system access and as such it would be useful if it could be done asynchronously.

You're right, I thought about the same issue regarding command substitution (that's also async in nature). The solution is not an easy one, because all the parser has to become asyncronous... Or have you some brighter idea than me?

[jhanssen]

you are supposed to return q single string '/tmp/a /tmp/b'

That's what I thought but it seems I end up with just one argument of /tmp/a if I do that. It looks like the reason for this is that unquote() in quote-removal.js only considers the first element of the unquoted array.

Or have you some brighter idea than me?

No, I think you're right in that the parser has to become asynchronous and I agree that it's not a simple change.

Thanks.

[piranna]

if you receive as argument '/tmp/*' (supposing you resolve the globbing to two files: '/tmp/a' and '/tmp/b') you are supposed to return q single string '/tmp/a /tmp/b'

Why to return a formatted string? Wouldn't it be better and simpler to return an array instead? It could be converted to an string too...

The solution is not an easy one, because all the parser has to become asyncronous...

I would move towards that path... :-)

Or have you some brighter idea than me?

Async / await, maybe?

[nfischer]

you are supposed to return q single string '/tmp/a /tmp/b'.

Why not return an array, such as ['/tmp/a', '/tmp/b']? This sounds like a more convenient API--otherwise users need to manually escape spaces, which is easy to miss. Also, modules such as node-glob already return arrays.

[parro-it]

Why not return an array, such as ['/tmp/a', '/tmp/b']

Right, I'll made such change... or someone want to do a PR 😸 ?

Anyone know of a good npm module to escape file names? There could be more to escape than spaces, e.g.:

Async / await, maybe?

I ❤️ async/await, it could greatly simplify asynchronous code... but I don't wnat to setup a transpilation, so that mean that we can support only node > 7.

@nfischer what about cash? Which version of node it support?

[dthree]

I don't mind bumping up the support to 7. Cash is a front-end library, so it doesn't drag monolithic projects with it that can't upgrade so easily.

[parro-it]

I don't mind bumping up the support to 7. Cash is a front-end library, so it doesn't drag monolithic projects with it that can't upgrade so easily.

:+1: @dthree, so I will start to use syntax construct only available in node >= 7.4 in the process of introducing async behavior in the parser.

[nfischer]

Anyone know of a good npm module to escape file names? There could be more to escape than spaces,

I don't see why this module needs to escape things for the shell. Did you mean parsing?

Most modules would prefer the un-escaped file names (e.g. fs.existsSync('file with spaces.txt'), glob.sync('path;with(special*chars.txt')). If we return escaped words like file\ with\ spaces.txt, then we're just making trouble when using this in cash.

A ShellEscape() function is only useful if you're sending commands to an external shell (like a bash instance). We had lengthy discussion over at shelljs/shelljs#143, and I was persuaded to provide a better solution that required no escaping (shelljs/shelljs#524)

[parro-it]

I don't see why this module needs to escape things for the shell. Did you mean parsing?

No, I really mean escape, and that's not to pass escaped version of the file names to child process, it's because I have to apply field splitting to the result of globbing, because file names has to be joined in a single string and later pass to child process as an array.

But after more thorough thinking I'm not sure that's really necessary...

[nfischer]

file names has to be joined in a single string and later pass to child process as an array.

But after more thorough thinking I'm not sure that's really necessary...

I don't see why it would be necessary. To me, it sounds like the worse option: we would be requiring users to use unsafe APIs like child_process.exec() instead of child_process.execFile() to process the output. If you see a reason why it would be necessary, please explain.