@nfischer: It looks like this was implemented on top of #344, is that correct? If so, it looks like we'll have to do some git-fu to fix it.

@nfischer: Maybe I'm missing something, but how is returning a ShellString any different from returning a normal string?

Also, what are your thoughts on converting them to ShellStrings inside common.wrap, vs. the command itself.

@ariporad Convering in common.wrap is fine with me, as long as it works.

JavaScript has strings and Strings. The capital String is an object. The two are very similar (the same methods). They differ in type though, and that's significant. Because Strings are objects, we can assign properties to them without modifying the prototype (and that's the important part). ShellStrings are intentionally very similar to Strings. We can easily add features though, like .to(), pipes, stderr, code, etc

This still isn't ready to merge. I'll ping this thread when it's ready for review.

@ariporad Please take a look at my implementation. This is quite a big refactor, but I think it has some nice benefits.

• This fixes Don't modify string prototype #159 (unfortunately, this breaks backwards compatibility, but I think it's worth it)
• This will make it easy to add support for pipes, once everything is common.wraped (blocked by fix(verbose): verbose-style logging is consistent #362)
• This makes sure exec() (synchronous) returns a ShellString, which allows users to write things like exec('git status').to('file.txt') instead of exec('git status').stdout.to('file.txt'). This makes the second syntax actually invalid, and it also removes the deprecated output attribute (we can add that back pretty easily though if it's important).

We can also consider extending this type to have a .forEach() method that iterates over each line in the standard output. Ex:

cat('file.txt').forEach(function (line) {
  echo("  " + line);
}); // indent every line of the file in stdout

This might make it easy to refactor ls() to return a ShellString, and still allow users to iterate over its output. This hasn't been implemented yet, however, and probably should be a separate PR.

@nfischer: Is it possible to convert the result to a ShellString in common.wrap? Otherwise, LGTM!

@ariporad I thought about putting it in common.wrap(). The issue is that some commands return arrays (i.e. ls()), some return nothing (i.e. cp()), and some probably should return plain strings (like tempdir()). With some cleverness, we might be able to refactor for that, but it might just compliment things more (like an argument {idx: 1, shellstring: true}).

Ok.

LGTM!

Ari

On Fri, Feb 19, 2016 at 3:25 PM, Nate Fischer notifications@github.com
wrote:

@ariporad https://github.com/ariporad I thought about putting it in
common.wrap(). The issue is that some commands return arrays (i.e. ls()),
some return nothing (i.e. cp()), and some probably should return plain
strings (like tempdir()). With some cleverness, we might be able to
refactor for that, but it might just compliment things more (like an
argument {idx: 1, shellstring: true}).

—
Reply to this email directly or view it on GitHub
#360 (comment).

@ariporad Ok, the issues should be resolved now. LGTM, feel free to merge

@ariporad I went ahead and added some of these methods to the array returned by ls(), since I think this is a good direction to go. I left it as a separate commit for now, so it should be easier for you to find those changes. Let me know what you think.

If everything looks good, feel free to squash the changes together (or not, doesn't matter to me), and merge.

After this is merged, I can redo #269 so that each ShellString also has a code attribute (so it's similar to the object exec() returns).

@nfischer: One small nit, but it's not that big a deal, so I'll say LGTM.

As for the code prop, I say that we should just make error() return the code, because everything returns stderr.

Merge at will.