-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Conversation
Pull Request Test Coverage Report for Build 1431
💛 - Coveralls |
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.
Nice! The truffle-debugger stuff is rather clean!
Approving this but I would like to strongly encourage you to split Session.setOrClearBreakpoint
into two methods. (I just think it looks better for the interface)
else | ||
{ | ||
return this.dispatch(controller.removeBreakpoint(breakpoint)); | ||
} |
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.
I kinda think this should be two separate methods
{ | ||
config.logger.log(`Breakpoint removed at ${locationMessage}.`); | ||
} | ||
return; |
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.
Note for later: you might consider splitting this up into multiple functions that could live in debug-utils. But no need to do it now; I think you'll find yourself naturally wanting that in the course of any follow-up work.
splitArgs=cmd.trim().split(/ +/).slice(1); | ||
debug("splitArgs %O",splitArgs); | ||
|
||
//warning: this bit *alters* cmd! |
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.
isn't that kind of annoying?
else | ||
{ | ||
config.logger.log(`Breakpoint removed at ${locationMessage}.`); | ||
} |
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.
Ah so I just tried this feature out and realized something that might be nice: an extra blank line after these added/removed messages.
This looks a bit cluttered:
debug(develop:0x91c817a1...)> b SquareLib:5
Breakpoint added at line 5 in SquareLib.sol.
debug(develop:0x91c817a1...)> b +10
Breakpoint added at line 23.
debug(develop:0x91c817a1...)> B +10
Breakpoint removed at line 23.
Compared with:
debug(develop:0x91c817a1...)> b SquareLib:5
Breakpoint added at line 5 in SquareLib.sol.
debug(develop:0x91c817a1...)> b +10
Breakpoint added at line 23.
debug(develop:0x91c817a1...)> B +10
Breakpoint removed at line 23.
}; | ||
|
||
session.continueUntil(breakpoint); | ||
session.setOrClearBreakpoint(breakpoint,true); |
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.
Missed this!
let breakpointStopped = false; | ||
|
||
session.setOrClearBreakpoint(breakpoint,true); |
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.
Also this!
…e breakpoint command from the add breakpoint command.
…r new breakpoint code.
f788168
to
737291b
Compare
This branch expands the functionality of breakpoints as requested in #1276. Breakpoints can now be set on a line of the user's choice in a source file of their choice; they can also be set at the current location as before. Breakpoints are no longer toggled with
b
but rather set withb
and cleared withB
. A breakpoint set withb
with no arguments is set at the current location; this is the current location within the line, like the old breakpoints. Breakpoints may also be set at a given line (including in another source file), which may also be specified relative to the current line; these breakpoints will stop execution at the first encountered location within that line. Breakpoints in another source file need only have enough of the filename specified to unambiguously identify it; if what's given is ambiguous, the user will be prompted with a list of possibilities.This branch does not include expanded functionality for
?
.