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.

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

src: read break_node_first_line from the inspect options #28034

Closed

Conversation

@MarshallOfSound
Copy link
Member

commented Jun 2, 2019

There are cases where the debug_options() on the env are different to the options that were passed into inspector::Agent. This is particularly true in Electrons case where we pass the agent a real parsed NodeOptions but give the env an empty one.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@nodejs-github-bot

This comment has been minimized.

@BridgeAR BridgeAR requested review from addaleax and bnoordhuis Jun 4, 2019

@bnoordhuis
Copy link
Member

left a comment

The change itself LGTM but it needs a rebase. This section changed quite a bit but I think it's written like this now:

if (inspector_agent_->options().break_node_first_line) {

@MarshallOfSound MarshallOfSound force-pushed the MarshallOfSound:use-agent-options branch from e640735 to f459ef3 Jun 19, 2019

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

Sorry for the delay on this @bnoordhuis , rebased and updated 👍

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

Is it reasonable to write a test for this?

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

src: read break_node_first_line from the inspect options
There are cases where the debug_options() on the env are
different to the options that were passed into inspector::Agent.

@MarshallOfSound MarshallOfSound force-pushed the MarshallOfSound:use-agent-options branch from f459ef3 to 633b385 Jul 22, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

@jasnell @addaleax @BridgeAR Still looks good to you? This got a small change since the last CI and the approvals.

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

@jasnell @addaleax @BridgeAR Still looks good to you? This got a small change since the last CI and the approvals.

Oh, wait, it was just a rebase, I think. Never mind.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Landed in 4208158.

@Trott Trott closed this Jul 30, 2019

Trott added a commit to Trott/io.js that referenced this pull request Jul 30, 2019
src: read break_node_first_line from the inspect options
There are cases where the debug_options() on the env are
different to the options that were passed into inspector::Agent.

PR-URL: nodejs#28034
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@MarshallOfSound MarshallOfSound deleted the MarshallOfSound:use-agent-options branch Jul 30, 2019

targos added a commit that referenced this pull request Aug 2, 2019
src: read break_node_first_line from the inspect options
There are cases where the debug_options() on the env are
different to the options that were passed into inspector::Agent.

PR-URL: #28034
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR BridgeAR referenced this pull request Aug 6, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
src: read break_node_first_line from the inspect options
There are cases where the debug_options() on the env are
different to the options that were passed into inspector::Agent.

PR-URL: nodejs#28034
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
src: read break_node_first_line from the inspect options
There are cases where the debug_options() on the env are
different to the options that were passed into inspector::Agent.

PR-URL: nodejs#28034
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.