-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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.
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
debugger: introduce exec method for debugger #1491
Conversation
442a8bf
to
5d32f5d
Compare
@@ -1575,6 +1575,17 @@ Interface.prototype.repl = function() { | |||
this.repl.displayPrompt(); | |||
}; | |||
|
|||
Interface.prototype.exec = function(code) { | |||
var self = this; | |||
this.debugEval(code, null, null, function (err, result) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor style issue: no space after function
keyword.
LGTM sans style nit. |
thanks @bnoordhuis |
39fa227
to
a358dee
Compare
Curious: down the line is there interest in moving to a more python pdb-style debugger, where the interface is a REPL by default and will evaluate any expression inline, but still respond to debugging commands? |
@chrisdickinson +1 this tripped me up when I first started playing with Node |
@chrisdickinson hope there is an |
ping anyone? |
Not sure this is the best approach. I've recently experimented with the debugger for the first time, and was a bit surprised it wasn't a real repl. How about actually making it a repl and adding all debug related commands to the existing list of commands shown in the repl with This would of course be an incompatible change, but having all commands prefixed with a dot would be my preferred method. Maybe add a short print to refer to |
I use debugger heavily, and most commands I use are |
@silverwind the |
I think we should take a page from python's book and allow those commands unprefixed, but if the line isn't a known command treat it as a repl utterance.
|
That one where
|
One way around this variable name problem could be to require a semicolon to access local variables, so |
@nodejs/collaborators ... anyone have any idea on the status of this? Is it still something that's wanted? /cc @JacksonTian |
yes, repl is ugly. |
@JacksonTian can you at least make it |
ok. |
@silverwind Implement |
/\d/, /\d/, /\d/, /\d/, /\d/ | ||
]); | ||
|
||
// Execute | ||
addTest('exec("process.title")', [ | ||
/iojs/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to be node
now ;)
@@ -949,6 +950,12 @@ Interface.prototype.controlEval = function(code, context, filename, callback) { | |||
} | |||
} | |||
|
|||
// exec process.title => exec("process.title"); | |||
var match = code.match(/exec\s+([^\n]*)/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably use an anchoring match (i.e. /^\s*exec
) so it won't get confused by e.g. "exec exec exec"
as the input.
EDIT: Can you add a regression test for that while you're at it? Thanks.
5cdde8e
to
1031ab0
Compare
Thanks @bnoordhuis , I updated the commit. |
// execute expression | ||
Interface.prototype.exec = function(code) { | ||
var self = this; | ||
this.debugEval(code, null, null, function(err, result) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use an arrow function callback and get rid of self
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR was submitted long time ago : ). updated.
In debugger, the usage of `repl` very ugly. I'd like there is a `p` like gdb. So the `exec` is coming. Usage: ``` $ ./iojs debug ~/git/node_research/server.js < Debugger listening on port 5858 connecting to 127.0.0.1:5858 ... ok break in /Users/jacksontian/git/node_research/server.js:1 > 1 var http = require('http'); 2 3 http.createServer(function (req, res) { debug> exec process.title /Users/jacksontian/git/io.js/out/Release/iojs debug> ``` And the `repl`: ``` debug> repl Press Ctrl + C to leave debug repl > process.title '/Users/jacksontian/git/io.js/out/Release/iojs' debug> (^C again to quit) ``` The enter and leave debug repl is superfluous.
1031ab0
to
dbeac9a
Compare
Thanks, LGTM. |
LGTM2. I can't start the CI though, seems to be down at the moment. |
And it's up again: https://ci.nodejs.org/job/node-test-pull-request/767/ |
In debugger, the usage of `repl` very ugly. I'd like there is a `p` like gdb. So the `exec` is coming. Usage: ``` $ ./iojs debug ~/git/node_research/server.js < Debugger listening on port 5858 connecting to 127.0.0.1:5858 ... ok break in /Users/jacksontian/git/node_research/server.js:1 > 1 var http = require('http'); 2 3 http.createServer(function (req, res) { debug> exec process.title /Users/jacksontian/git/io.js/out/Release/iojs debug> ``` And the `repl`: ``` debug> repl Press Ctrl + C to leave debug repl > process.title '/Users/jacksontian/git/io.js/out/Release/iojs' debug> (^C again to quit) ``` The enter and leave debug repl is superfluous. R-URL: #1491 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Failures in CI look unrelated. Will get this landed. |
Landed in b33e9da |
In debugger, the usage of `repl` very ugly. I'd like there is a `p` like gdb. So the `exec` is coming. Usage: ``` $ ./iojs debug ~/git/node_research/server.js < Debugger listening on port 5858 connecting to 127.0.0.1:5858 ... ok break in /Users/jacksontian/git/node_research/server.js:1 > 1 var http = require('http'); 2 3 http.createServer(function (req, res) { debug> exec process.title /Users/jacksontian/git/io.js/out/Release/iojs debug> ``` And the `repl`: ``` debug> repl Press Ctrl + C to leave debug repl > process.title '/Users/jacksontian/git/io.js/out/Release/iojs' debug> (^C again to quit) ``` The enter and leave debug repl is superfluous. R-URL: #1491 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Is this something we would want in LTS? /cc @nodejs/lts |
hmmm... good question. lts-agenda label applied. |
lts wg has agreed this poses little risk. Added |
In debugger, the usage of `repl` very ugly. I'd like there is a `p` like gdb. So the `exec` is coming. Usage: ``` $ ./iojs debug ~/git/node_research/server.js < Debugger listening on port 5858 connecting to 127.0.0.1:5858 ... ok break in /Users/jacksontian/git/node_research/server.js:1 > 1 var http = require('http'); 2 3 http.createServer(function (req, res) { debug> exec process.title /Users/jacksontian/git/io.js/out/Release/iojs debug> ``` And the `repl`: ``` debug> repl Press Ctrl + C to leave debug repl > process.title '/Users/jacksontian/git/io.js/out/Release/iojs' debug> (^C again to quit) ``` The enter and leave debug repl is superfluous. R-URL: #1491 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
In debugger, the usage of `repl` very ugly. I'd like there is a `p` like gdb. So the `exec` is coming. Usage: ``` $ ./iojs debug ~/git/node_research/server.js < Debugger listening on port 5858 connecting to 127.0.0.1:5858 ... ok break in /Users/jacksontian/git/node_research/server.js:1 > 1 var http = require('http'); 2 3 http.createServer(function (req, res) { debug> exec process.title /Users/jacksontian/git/io.js/out/Release/iojs debug> ``` And the `repl`: ``` debug> repl Press Ctrl + C to leave debug repl > process.title '/Users/jacksontian/git/io.js/out/Release/iojs' debug> (^C again to quit) ``` The enter and leave debug repl is superfluous. R-URL: #1491 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
In debugger, the usage of `repl` very ugly. I'd like there is a `p` like gdb. So the `exec` is coming. Usage: ``` $ ./iojs debug ~/git/node_research/server.js < Debugger listening on port 5858 connecting to 127.0.0.1:5858 ... ok break in /Users/jacksontian/git/node_research/server.js:1 > 1 var http = require('http'); 2 3 http.createServer(function (req, res) { debug> exec process.title /Users/jacksontian/git/io.js/out/Release/iojs debug> ``` And the `repl`: ``` debug> repl Press Ctrl + C to leave debug repl > process.title '/Users/jacksontian/git/io.js/out/Release/iojs' debug> (^C again to quit) ``` The enter and leave debug repl is superfluous. R-URL: #1491 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
In debugger, the usage of
repl
very ugly. I'd like there is ap
like gdb. So the
exec
is coming.Usage:
And the
repl
:The enter and leave debug repl is superfluous.