Communication enhancements #13
Conversation
|
I'll take a look tomorrow! I'm interested to see how offliner works, a review seems a good/useful way to start looking into it. |
| * @method _checkForActivationPending | ||
| * @private | ||
| */ | ||
| Offliner.prototype._checkForActivationPending = function () { |
There was a problem hiding this comment.
Nit: Instead of defining another function, you could directly use _activate (which does the same thing).
|
Looks good to me, only nits. |
| @@ -228,6 +228,9 @@ | |||
| case 'xpromise': | |||
| this._receiveCrossPromise(msg.id, msg.order); | |||
| break; | |||
| case 'checkForActivationPending': | |||
There was a problem hiding this comment.
Nit: I think the correct wording (here and elsewhere) is check for pending activation, but I'm not totally sure 😄
There was a problem hiding this comment.
Nit: I think the correct wording (here and elsewhere) is check for pending activation, but I'm not totally sure 😄
They're both reasonably correct! "Check for activation pending" can mean "check for an activation that is pending," while "check for pending activation" can mean "check for a pending activation." And those are the same thing. In both cases, the word "pending" is being used as an adjective, to modify the noun "activation."
However, "check for activation pending" could be interpreted to mean "check that the thing that is pending is an activation," which would be incorrect here. And the most important part of the sentence is "pending," as it's key to the check: whether or not there is an activation, the key is to check whether or not the activation (if any) is pending.
So I would probably go with "checkForPendingActivation".
There was a problem hiding this comment.
Although now that I read through the rest of the code and see other uses, I'd stick with "checkForActivationPending", as it reuses the order of the words in symbols like "isActivationPending" and "activation-pending".
|
Looks good to me too. Like Marco, I don't have a good sense of this codebase yet, but the changes seem straightforward. |
This closes #8 , closes #7 , closes #6 and closes #5 .
@mykmelez or @marco-c , do you want to take a look for a second opinion before merging? Thanks!
PS: You should pay attention to files under
/srcas everything else is autogenerated.