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

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

Merged
merged 4 commits into from
Nov 20, 2018

Conversation

kjin
Copy link
Contributor

@kjin kjin commented Sep 27, 2018

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
Contributor

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

Copy link
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?

Copy link
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
Contributor Author

kjin commented Oct 3, 2018

@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
Copy link
Contributor Author

kjin commented Oct 19, 2018

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

@DominicKramer
Copy link
Contributor

@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.

Copy link
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
Contributor

@ofrobots Ok will do.

@DominicKramer
Copy link
Contributor

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.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants