[WIP] exec async bug? - #182

I saw your changes and I think you don't need to change any logic there, if you just change the default value of async:

  // Callback is defined instead of options.
  if (typeof options === 'function') {
    callback = options;
    options = { async: false };
  }

Then it will only be true if there's a callback specified

  // Callback is defined with options.
  if (typeof options === 'object' && typeof callback === 'function') {
    options.async = true;
  }

I believe this is what was intended, but it has a typo on the initialisation of the async property.

@thegiantbeast I don't think that your first change would be beneficial. Then every time you provide just a callback, it will be run syncronously. Which is not what the docs imply.

If you provide options like {async:false} and a callback the bug will still persist.

ok so I started from scratch an made a new PR in #184

@vise890 I think you should review your logic, your latest change wouldn't work either.

options.async = options.async || true;

You won't be able to set async as false as false || true will always return true...

I checked the code again and the logic is right (ignore my previous comment about the typo).
So if you don't pass a callback (either on 2nd or 3rd param) async will always be false, but if you do, it means it will run asynchronously and it's not configurable because that's how it's supposed to work (the sync method doesn't even support a callback).

What are you trying to achieve?

You're right in pointing out that my logic is wrong.

What I'm trying to achieve is that when you pass {async:false} as options
and a callback the task will be run synchronously.

If that's not possible then you shouldn't say that options.async is true
by default if you provide a callback. It made me believe that there was a
possibility to set it to false. You should say that options.async is true
when a callback is provided and that any value passed is ignored.

Here's the specific point in the docs I'm referring to BTW:
http://documentup.com/arturadib/shelljs#command-reference/exec-command-options-callback

I'll have another look tomorrow.

@vise890 https://github.com/vise890 I think you should review your logic,
your latest change wouldn't work either.

options.async = options.async || true;

You won't be able to set async as false as false || true will always return
true...

I checked the code again and the logic is right (ignore my previous comment
about the typo).
So if you don't pass a callback (either on 2nd or 3rd param) async will
always be false, but if you do it means it will run asynchronously and it's
not configurable because that's how it's supposed to work (the sync method
doesn't even support a callback).

What are you trying to achieve?

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

I think you misunderstood what is said (maybe it needs some clarification), but the documentation what's trying to say is that in case of a callback you don't need to see async as it's set automatically for you.

And the option is there for the case where you don't have/need a callback but you still want to run the command asynchronously, ie .exec('ls /', {async: true}).

But the feature is correct and the documentation is also "correct", but maybe it should clarify a little bit better.

Ok have you had a look at #185 ? I think the docs aren't very clear when they state:

[async] Defaults to true if a callback is provided.

To me, saying that it "defaults" to true, implies that you can still set it... I don't know, maybe it's just me.

Yes, I understand what you're saying and that's why I said documentation needs some clarification (maybe replace default with:

[async] automatically sets to true if a callback is provided.

I think this will explain correctly the functionality of it.