Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

debugger: improve ESRCH error message #1863

Closed
wants to merge 1 commit into from
Closed

debugger: improve ESRCH error message #1863

wants to merge 1 commit into from

Conversation

JacksonTian
Copy link
Contributor

When use iojs debug -p <pid> with an invalid pid,
the debugger print internal error message also,
it not enough smart.

@Fishrock123
Copy link
Contributor

SGTM..

bnoordhuis
bnoordhuis reviewed Jun 1, 2015
View reviewed changes
lib/_debugger.js
@@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

Can you use const here?

@JacksonTian
Copy link
Contributor Author

Thank you. @Fishrock123 @bnoordhuis @thefourtheye

@bnoordhuis
Copy link
Member

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/749/

@JacksonTian
Copy link
Contributor Author

The smart os has a timeout error. It seems not related. And the raspbian is so slowly.

在 2015年6月2日，下午3:57，Ben Noordhuis notifications@github.com 写道：

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/749/ https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/749/
—
Reply to this email directly or view it on GitHub #1863 (comment).

@bnoordhuis
Copy link
Member

LGTM but can I have one more LGTM from @nodejs/collaborators? Perhaps it would be good to have a regression test for this.

Fishrock123
Fishrock123 reviewed Jun 3, 2015
View reviewed changes
lib/_debugger.js
@@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be stderr then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or simply, console.error?

@Fishrock123
Copy link
Contributor

A question, but LGTM, tests are always nice. :)

@indutny
Copy link
Member

indutny commented Jun 3, 2015

LGTM, with one nit from @Fishrock123

@JacksonTian
Copy link
Contributor Author

Add the regression test.

yosuke-furukawa
yosuke-furukawa reviewed Jun 5, 2015
View reviewed changes
lib/_debugger.js
process._debugProcess(pid);
} catch (e) {
if (e.code === 'ESRCH') {
console.error('Target process: ' + pid + ' doesn\'t exist.');
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to use escape \ enclosed by " or `.

console.error(`Target process: ${pid} doesn't exist.`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 notifications@github.com 写道：

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.');
    We don't need to use escape \ enclosed by " or `.

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, We can use template string literals. We switched closure-linter to eslint.
https://github.com/nodejs/io.js/blob/master/.eslintrc#L7

@JacksonTian
debugger: improve ESRCH error message
2cdc830 
When use `iojs debug -p <pid>` with an invalid pid,
the debugger print internal error message also,
it not enough smart.
@JacksonTian
Copy link
Contributor Author

@yosuke-furukawa used the template string.