Skip to content
This repository was archived by the owner on Apr 3, 2024. It is now read-only.

fix: use well-formatted URLs when setting breakpoints#535

Merged
DominicKramer merged 4 commits intogoogleapis:masterfrom
kjin:node-11-fix
Nov 20, 2018
Merged

fix: use well-formatted URLs when setting breakpoints#535
DominicKramer merged 4 commits intogoogleapis:masterfrom
kjin:node-11-fix

Conversation

@kjin
Copy link
Copy Markdown
Contributor

@kjin kjin commented Sep 27, 2018 •
edited
Loading

Newer versions of Node will require that URLs be well-formatted when setting breakpoints (and will also return well-formatted URLs), as a result of nodejs/node#22223 (this will also affect versions of Node to which that PR will be backported). Enforcing well-formatted URLs will break the Debug Agent, so this PR addresses that.

If tests pass for current versions of Node, I think this should be merged now (rather than wait for Node 11).

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@kjin kjin requested a review from a team September 27, 2018 01:24
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 27, 2018
@ghost ghost assigned kjin Sep 27, 2018
@kjin kjin removed the request for review from a team September 27, 2018 18:27
@ghost ghost assigned JustinBeckwith Oct 3, 2018
@JustinBeckwith
Copy link
Copy Markdown
Contributor

JustinBeckwith commented Oct 3, 2018

@kjin @googleapis/node-team this is just kinda hangin around :) Who should be reviewing this?

soldair
soldair approved these changes Oct 3, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@soldair soldair left a comment

Choose a reason for hiding this comment

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

seems good.
is there a way to add a test for this?

DominicKramer
DominicKramer approved these changes Oct 3, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@DominicKramer DominicKramer left a comment

Choose a reason for hiding this comment

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

Thanks for addressing this. Looks good once the tests are passing. I agree it would be nice if there was a way to test this.

@kjin
Copy link
Copy Markdown
Contributor Author

kjin commented Oct 3, 2018 •
edited
Loading

@JustinBeckwith This PR needs work, I've been busy with other things so haven't had a chance to update it.

@soldair @DominicKramer Currently, many tests will fail on Node master (or v10.x-staging) without this change... not sure if we need to explicitly test this.

@kjin kjin force-pushed the node-11-fix branch 3 times, most recently from a52b5d1 to 0d35fad Compare October 3, 2018 23:38
@kjin
Copy link
Copy Markdown
Contributor Author

kjin commented Oct 19, 2018

@soldair @DominicKramer I'd appreciate a look on the newest commit. Thanks!

@kjin kjin mentioned this pull request Oct 24, 2018
Merged
@DominicKramer
Copy link
Copy Markdown
Contributor

DominicKramer commented Nov 19, 2018

@kjin I've updated this PR to merge your approach and the approach I took in PR #560. Could you take a look to see if it looks good to land? Thanks.

ofrobots
ofrobots approved these changes Nov 19, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

Your change LGTM. If the tests are good, go ahead and land this.

@DominicKramer
Copy link
Copy Markdown
Contributor

DominicKramer commented Nov 19, 2018

@ofrobots Ok will do.

@DominicKramer
Copy link
Copy Markdown
Contributor

DominicKramer commented Nov 19, 2018

This will be able to land after PR #583 lands.

@DominicKramer DominicKramer merged commit cefd4ae into googleapis:master Nov 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@ofrobots ofrobots ofrobots approved these changes

@soldair soldair soldair approved these changes

@DominicKramer DominicKramer DominicKramer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JustinBeckwith JustinBeckwith

@kjin kjin

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kjin @JustinBeckwith @DominicKramer @ofrobots @soldair @googlebot