fix(check): fix check via API not passing results to the callback #563
Conversation
lib/commands/on-complete.js
Outdated
| if (typeof callback !== 'function') { | ||
| originalErr = originalErr || callback; | ||
| originalErr = callback; |
There was a problem hiding this comment.
this change causing test failures.
As I understand it, the difference between the two only happens when:
callbackis provided but is not a functionoriginalErris also provided
When would that ever occur? Attempting to debug via the tests but not easy going due to the child processes
There was a problem hiding this comment.
Remove this change please, this is interferring with core functionalities.
RandomSeeded
force-pushed
the
nate/fixOnComplete
branch
from
07860d2 to
0cda205
Compare
Signed-off-by: RandomSeeded <[email protected]>
RandomSeeded
force-pushed
the
nate/fixOnComplete
branch
from
0cda205 to
b743696
Compare
| if (typeof callback !== 'function') { | ||
| originalErr = originalErr || callback; | ||
| results = results || originalErr; |
There was a problem hiding this comment.
@wzrdtales I removed the change from line 6 as you requested.
That said, I don't understand under what circumstances callback is provided yet is not a function.
I'm concerned about leaving line 6 in its original state because if callback is not provided and results are, then originalErr will contain the results and not an error.
There was a problem hiding this comment.
LMK if that doesn't make sense to you, not sure if I worded that as clearly as I could have...
There was a problem hiding this comment.
If there is an error, there will be no results though :)
There was a problem hiding this comment.
And yeah ok, the callback thing, that would need to fit with the condition if the callback is there or not, which is the only unique circumstance here.
There was a problem hiding this comment.
But if no callback is passed, that should be valid anyway, since you do not need the results even if they're existent, since you do not deliver them.
There was a problem hiding this comment.
Hah! That is true. Still seems a bit fragile to me but I'm OK with it if you are :)
|
Anything else you want here @wzrdtales or is this good to merge? |
|
hadn't yet time again, last week was quite bloated. will give this a look the next days. please hit me up again should you not receive an answer by friday :) |
|
@wzrdtales pinging as requested :) |
|
Thanks for pinging me :) Looks good so far merging |
|
Sry haven't pushed out a release yet @RandomSeeded . Last days and weeks have been really time consuming for me. |
|
No worries @wzrdtales! I just appreciate the work you've done here ❤️ I am still working on a new branch which will have any remaining changes I'm hoping to get merged in (mostly to the mongodb driver) but there's no urgency; I'll just use a small fork in the meantime. |
Signed-off-by: RandomSeeded [email protected]