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

inspector: restore --debug-brk alias #12580

Closed
wants to merge 2 commits into from
Closed

inspector: restore --debug-brk alias #12580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85bf8d0
inspector: restore --debug-brk alias
sam-github Apr 22, 2017
698cda6
fixup! inspector: restore --debug-brk alias
sam-github Apr 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions src/node_debug_options.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,15 +97,13 @@ bool DebugOptions::ParseOption(const std::string& option) {
if (option_name == "--inspect") {
debugger_enabled_ = true;
enable_inspector = true;
} else if (option_name == "--inspect-brk") {
} else if (option_name == "--inspect-brk" || option_name == "--debug-brk") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make sure that --debug-brk without --inspect still fails?

debugger_enabled_ = true;
enable_inspector = true;
wait_connect_ = true;
} else if ((option_name != "--debug-port" &&
option_name != "--inspect-port") ||
!has_argument) {
// only other valid possibility is --inspect-port,
// which requires an argument
} else if (option_name == "--inspect-port" || option_name == "--debug-port") {
Copy link
Member

Choose a reason for hiding this comment

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

What should happen if both --inspect-port and --debug-port are used? Currently the way this is written, whichever one is used last in the command line takes precedence. However, it would be an obvious error if both are used, especially if they specify different values, so it should likely throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd agree its a bit weird that literally every debug option can be followed with an optional host:port spec, but that's how it is/was, and its not specific to --debug-brk.

For example, --inspect-port=9999 --inspect=9876 --inspect-brk=8888 is currenlty supported (does the obvious thing, last wins), would you propose that become an error? I can see the argument for that, and the argument against it, but it doesn't seem to me that if --debug-brk is added as an alias of --inspect-brk that it should get special cased.

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I would rather --inspect-port=9999 --inspect=9876 --inspect-brk-8888 result in an error because there's obviously no way to correctly interpret or implement the users intent. Essentially, if the host:port is specified multiple times, there should be an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, but making currently allowed syntax invalid is not related to the problem this PR addresses (and would be semver-major).

if (!has_argument) return false;
} else {
return false;
}

Expand Down
2 changes: 1 addition & 1 deletion src/node_debug_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class DebugOptions {
#if HAVE_INSPECTOR
bool inspector_enabled_;
#endif // HAVE_INSPECTOR
bool wait_connect_;
bool wait_connect_; // --inspect-brk
bool http_enabled_;
std::string host_name_;
int port_;
Expand Down