debugger: improve ESRCH error message #1863
Conversation
|
SGTM.. |
| @@ -1639,7 +1639,16 @@ Interface.prototype.trySpawn = function(cb) { | |||
| } else if (this.args.length === 3) { | |||
| // `node debug -p pid` | |||
| if (this.args[1] === '-p' && /^\d+$/.test(this.args[2])) { | |||
| process._debugProcess(parseInt(this.args[2], 10)); | |||
| var pid = parseInt(this.args[2], 10); | |||
JacksonTian
force-pushed
the
debug_process
branch
from
58827e7
to
bf9bffa
Compare
|
Thank you. @Fishrock123 @bnoordhuis @thefourtheye |
|
The smart os has a timeout error. It seems not related. And the raspbian is so slowly.
|
|
LGTM but can I have one more LGTM from @nodejs/collaborators? Perhaps it would be good to have a regression test for this. |
| @@ -1705,7 +1714,7 @@ Interface.prototype.trySpawn = function(cb) { | |||
| // If it's failed to connect 10 times then print failed message | |||
| if (connectionAttempts >= 10) { | |||
| self.stdout.write(' failed, please retry\n'); | |||
There was a problem hiding this comment.
Shouldn't this be stderr then?
There was a problem hiding this comment.
Or simply, console.error?
|
A question, but LGTM, tests are always nice. :) |
|
LGTM, with one nit from @Fishrock123 |
JacksonTian
force-pushed
the
debug_process
branch
from
bf9bffa
to
cec83f7
Compare
|
Add the regression test. |
| process._debugProcess(pid); | ||
| } catch (e) { | ||
| if (e.code === 'ESRCH') { | ||
| console.error('Target process: ' + pid + ' doesn\'t exist.'); |
There was a problem hiding this comment.
We don't need to use escape \ enclosed by " or `.
console.error(`Target process: ${pid} doesn't exist.`);There was a problem hiding this comment.
Didn’t see template string be used in node core, so I don’t use it. If it’s recommend, I will replace it.
在 2015年6月5日，上午11:21，Yosuke Furukawa [email protected] 写道：
In lib/_debugger.js #1863 (comment):
@@ -1639,7 +1639,16 @@ Interface.prototype.trySpawn = function(cb) {
} else if (this.args.length === 3) {
//node debug -p pid
if (this.args[1] === '-p' && /^\d+$/.test(this.args[2])) {
process._debugProcess(parseInt(this.args[2], 10));const pid = parseInt(this.args[2], 10);try {process._debugProcess(pid);} catch (e) {if (e.code === 'ESRCH') {console.error('Target process: ' + pid + ' doesn\'t exist.');console.error(
Target process: ${pid} doesn't exist.);
—
Reply to this email directly or view it on GitHub https://github.com/nodejs/io.js/pull/1863/files#r31785321.
There was a problem hiding this comment.
Yes, We can use template string literals. We switched closure-linter to eslint.
https://github.com/nodejs/io.js/blob/master/.eslintrc#L7
When use `iojs debug -p <pid>` with an invalid pid, the debugger print internal error message also, it not enough smart.
JacksonTian
force-pushed
the
debug_process
branch
from
cec83f7
to
2cdc830
Compare
|
@yosuke-furukawa used the template string. |
|
Yes. LGTM. |
|
Here's another test run: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/772/ - the last one failed because of style issues in the test case... |
When using `iojs debug -p <pid>` with an invalid pid, the debugger printed an internal error message because it wasn't smart enough to figure out that the target process didn't exist. Now it is. PR-URL: #1863 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Yosuke Furukawa <[email protected]>
|
Thanks @JacksonTian, landed in 81029c6. |
When use
iojs debug -p <pid>with an invalid pid,the debugger print internal error message also,
it not enough smart.