Skip to content

Attach to local process - launch.json update #8831

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

Merged
merged 6 commits into from
Nov 28, 2019

Conversation

kimadeline
Copy link

For #8384

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Appropriate comments and documentation strings in the code
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated
  • Test plan is updated as appropriate
  • package-lock.json has been regenerated by running npm install (if dependencies have changed)
  • The wiki is updated with any design decisions/details.

@codecov-io
Copy link

codecov-io commented Nov 27, 2019

Codecov Report

Merging #8831 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8831      +/-   ##
==========================================
+ Coverage   58.65%   58.68%   +0.02%     
==========================================
  Files         526      526              
  Lines       27212    27231      +19     
  Branches     4059     4064       +5     
==========================================
+ Hits        15962    15980      +18     
- Misses      10371    10372       +1     
  Partials      879      879
Impacted Files Coverage Δ
src/client/telemetry/index.ts 86.48% <ø> (ø) ⬆️
src/client/debugger/types.ts 100% <ø> (ø) ⬆️
src/client/debugger/extension/adapter/factory.ts 88.57% <100%> (+0.69%) ⬆️
...ugger/extension/hooks/childProcessAttachService.ts 100% <100%> (ø) ⬆️
src/client/telemetry/constants.ts 100% <100%> (ø) ⬆️
src/client/api.ts 92.85% <0%> (-0.48%) ⬇️
src/client/datascience/types.ts 100% <0%> (ø) ⬆️
src/client/datascience/jupyter/jupyterCommand.ts 97.1% <0%> (+0.04%) ⬆️
src/client/datascience/datascience.ts 23.83% <0%> (+0.44%) ⬆️
...lient/datascience/jupyter/kernels/kernelService.ts 56.77% <0%> (+1.67%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e75a71...52086fd. Read the comment docs.

@kimadeline kimadeline marked this pull request as ready for review November 27, 2019 21:06
const port = configuration.port ? configuration.port : 0;
const isAttach = configuration.request === 'attach';
const port = configuration.port ?? 0;
const processId = configuration.processId ?? 0;
Copy link

@DonJayamanne DonJayamanne Nov 28, 2019

Choose a reason for hiding this comment

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

Curious, if we have a check for 0 , then why not < 0
I think we can prevent user from entering 0 or -2 by using the json schema.
This way the code can validate whether is a number or not (deal with nullables, instead of 0 or not)

Copy link
Author

Choose a reason for hiding this comment

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

In the json schema port and processId are numbers, so there is already some validation going on. @karthiknadig's original logic was to set port's value to 0 if undefined since it isn't an accepted value anyway, I just applied it toprocessId as well.

@kimadeline kimadeline merged commit 7ee819a into microsoft:master Nov 28, 2019
@kimadeline kimadeline deleted the 8384-attach-launch-json branch November 28, 2019 21:47
@eoitaven
Copy link

eoitaven commented Dec 5, 2019

My VSCode don't create launch.json. I don't know why...

@lock lock bot locked as resolved and limited conversation to collaborators Dec 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants