Skip to content

Conversation

@mbillau
Copy link
Contributor

@mbillau mbillau commented Oct 17, 2013

Tried adding some positive confirmation message to bin/check_reqs, since currently it only prints errors. Not sure if this is the correct way to do this in Q.js, would appreciate any and all comments!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in your then() is basically turning Q.allSettled back into Q.all.

If one of the child promises fails, Q.all fails with that error. If they all pass, it returns the array of their fulfillment values. Therefore the logic you want is:

return Q.all([this.check_ant(), this.check_java(), this.check_android()]).then(function() {
    console.log('Looks like your environment fully supports cordova-android development!');
});

And that should behave identically to this code while being much simpler.

Also note that we should be calling check_java(), not passing it. That's an error in my original, apparently, and it should be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Branden, thanks. So .then() will only be called if none of the promises fail. I thought it was passing in the array of fulfillment values which is why I was then checking this array...but it definitely felt redundant. I made the changes (and also added -version to the java call since just calling 'java' will fail.)

@bshepherdson
Copy link
Contributor

LGTM. I'll merge these. FYI, I'm going to squash them into one commit, because the net change is a lot smaller than changing and then putting back and so on.

@bshepherdson
Copy link
Contributor

Done. ASFBot probably won't close this PR, since I rebased it. Can you please close it?

(Merged as 5ab11ed.)

@mbillau
Copy link
Contributor Author

mbillau commented Oct 18, 2013

Sure thing, thanks.

@mbillau mbillau closed this Oct 18, 2013
@mbillau mbillau deleted the CB-5117 branch October 24, 2013 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants