Update error message: output port at which port finder failed #178
Conversation
… open port Allows promise and callback versions of the api to both provide the port at which they failed at in their error message. This could also be done by changing the promise resolver to take the ports array but that seems superfluous.
|
review me @MasterOdin |
| } | ||
| else { | ||
| const msg = 'No open ports found in between '+ options.startPort + ' and ' + options.stopPort; | ||
| const msg = `Searching for an open port failed at port ${openPorts[0]}.`; |
There was a problem hiding this comment.
I think this makes the error message worse for the single port case as it's less informative, where it sort of sounds like the library only tested port 32773 and that failed, so here's an error, where it had checked several ports.
For the multiple port case, I think it's fine for the callback variant as you get a partial array of ports it could find, whereas for the promise variant I think it's worse as similar to the single port variant, you cannot tell that it had tested other ports before failing.
I think ideally the error message would be as it is for single port and then like "Could not find 3 open ports between ...." as then it still gives the range info.
There was a problem hiding this comment.
Yeah I agree. Whether we wanted to change the error message and how is basically what I wanted to get your feedback on.
Thanks again for helping out. It’s a lot more likely to change and get better with two heads.
|
Closing this. The right implementation should likely go where it says HERE below: internals.getPorts = function (count, options, callback) {
let lastPort = null;
_async.timesSeries(count, function(index, asyncCallback) {
if (lastPort) {
options.port = exports.nextPort(lastPort);
}
internals.getPort(options, function (err, port) {
if (err) {
// HERE: port is actually is defined as an array of ports, e.g. [3000, 3001, undefined]
// so we can provide a better error message, that fits
// like
// asyncCallback(`some better message that uses ${port} and maybe ${err}`);
// see the getPorts tests for where this idea came from.
//
// but probably just "Could not find 3 open ports between ...."
asyncCallback(err);
} else {
lastPort = port;
asyncCallback(null, port);
}
});
}, callback);
};It's tracked with #174 for now. |
Update error message: output port at which port finder failed
First attempt to modernize error messages for both callback and promise, and make them more accurate on failure.