Skip to content

Use python debug adapter when in experiment#8119

Merged
karthiknadig merged 13 commits into
microsoft:masterfrom
karthiknadig:newda
Oct 26, 2019
Merged

Use python debug adapter when in experiment#8119
karthiknadig merged 13 commits into
microsoft:masterfrom
karthiknadig:newda

Conversation

@karthiknadig
Copy link
Copy Markdown
Member

@karthiknadig karthiknadig commented Oct 21, 2019 •
edited
Loading

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Appropriate comments and documentation strings in the code
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated

@karthiknadig karthiknadig added the no-changelog No news entry required label Oct 21, 2019
@karthiknadig karthiknadig changed the title Use python debug adapter when in experiment WIP - Use python debug adapter when in experiment Oct 21, 2019
@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 21, 2019 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8119      +/-   ##
==========================================
+ Coverage   58.99%   59.02%   +0.02%     
==========================================
  Files         504      504              
  Lines       23105    23114       +9     
  Branches     3727     3744      +17     
==========================================
+ Hits        13631    13643      +12     
+ Misses       8597     8583      -14     
- Partials      877      888      +11
Impacted Files Coverage Δ
src/client/debugger/extension/types.ts 100% <ø> (ø) ⬆️
src/client/api.ts 92.85% <100%> (ø) ⬆️
src/client/debugger/extension/adapter/factory.ts 94.11% <100%> (+0.96%) ⬆️
src/client/datascience/webViewHost.ts 47.15% <0%> (-1%) ⬇️
src/datascience-ui/interactive-common/mainState.ts 50.76% <0%> (+3.83%) ⬆️
src/client/datascience/jupyter/jupyterNotebook.ts 10.43% <0%> (+4.39%) ⬆️

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 b323ce1...66c9649. Read the comment docs.

@karthiknadig karthiknadig force-pushed the newda branch 4 times, most recently from 13a43ee to 11cb1d1 Compare October 21, 2019 06:52
@karthiknadig karthiknadig changed the title WIP - Use python debug adapter when in experiment Use python debug adapter when in experiment Oct 21, 2019
@karthiknadig karthiknadig mentioned this pull request Oct 21, 2019
Closed
kimadeline
kimadeline suggested changes Oct 21, 2019
View reviewed changes
Copy link
Copy Markdown

@kimadeline kimadeline left a comment

Choose a reason for hiding this comment

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

The Python code looks good to me, but any Python code I didn't write looks good 🙈

Requesting changes for the cleanup that needs to be done in adapter/factory.unit.test.ts, other than that 👍

Comment thread pythonFiles/install_ptvsd.py
Comment thread src/client/debugger/extension/adapter/factory.ts
Comment thread src/client/debugger/extension/adapter/factory.ts
Comment thread src/client/debugger/extension/adapter/factory.ts Outdated
Comment thread src/test/debugger/extension/adapter/factory.unit.test.ts Outdated
Comment thread src/test/debugger/extension/adapter/factory.unit.test.ts Outdated
Comment thread src/test/mocks/vsc/extHostedTypes.ts Outdated
kimadeline
kimadeline approved these changes Oct 22, 2019
View reviewed changes
Copy link
Copy Markdown

@kimadeline kimadeline left a comment

Choose a reason for hiding this comment

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

You're a TS expert now 💯

ericsnowcurrently
ericsnowcurrently suggested changes Oct 22, 2019
View reviewed changes
Copy link
Copy Markdown

@ericsnowcurrently ericsnowcurrently 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 working on this, @karthiknadig! I've left a few comments to consider.

Also, it seems like this PR is packing in a number of different changes:

  • activating the experiment
  • moving away from requirements.txt for ptvsd
  • attach handling in the factory
  • other small things...

For the most part we should try to keep each PR focused, for the benefits of reviewability and revertability.

Comment thread pythonFiles/install_ptvsd.py
Comment thread pythonFiles/install_ptvsd.py
Comment thread pythonFiles/install_ptvsd.py
Comment thread pythonFiles/install_ptvsd.py
Comment thread pythonFiles/install_ptvsd.py
Comment thread pythonFiles/tests/debug_adapter/test_install_ptvsd.py
Comment thread pythonFiles/tests/debug_adapter/test_install_ptvsd.py
Comment thread src/client/debugger/extension/adapter/factory.ts
Comment thread src/client/debugger/extension/adapter/factory.ts
Comment thread src/client/debugger/extension/adapter/factory.ts
@karthiknadig karthiknadig reopened this Oct 23, 2019
@karthiknadig
Copy link
Copy Markdown
Member Author

karthiknadig commented Oct 25, 2019

I plan on merging this after the CI is clean. Some bad merge in master has the build hygiene failing everywhere.

  1. I have addressed all the comments. except for the one about requirements.txt and I have provided the reason why we should not use it for ptvsd.
  2. This is needed to make progress on the reload work.

@karthiknadig karthiknadig reopened this Oct 25, 2019
@karthiknadig karthiknadig reopened this Oct 25, 2019
@karthiknadig karthiknadig merged commit 93852c1 into microsoft:master Oct 26, 2019
@karthiknadig karthiknadig deleted the newda branch November 1, 2019 17:26
@lock lock Bot locked as resolved and limited conversation to collaborators Nov 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@kimadeline kimadeline kimadeline approved these changes

@ericsnowcurrently ericsnowcurrently ericsnowcurrently requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

no-changelog No news entry required

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@karthiknadig @codecov-io @ericsnowcurrently @kimadeline