Skip to content
This repository was archived by the owner on Aug 2, 2023. It is now read-only.

Refactor before fixing support for re-attach. #296

Merged
ericsnowcurrently merged 9 commits into from
Apr 4, 2018
Merged

Refactor before fixing support for re-attach. #296

ericsnowcurrently merged 9 commits into from
Apr 4, 2018

Conversation

@ericsnowcurrently
Copy link
Contributor

@ericsnowcurrently ericsnowcurrently commented Apr 3, 2018 •
edited
Loading

An alternative approach to fixing re-attach (in contrast to #287) requires separating some concerns that are currently mixed together in the code base. This PR does so.

@codecov-io
Copy link

codecov-io commented Apr 3, 2018 •
edited
Loading

Codecov Report

Merging #296 into master will increase coverage by 1.33%.
The diff coverage is 72.24%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #296      +/-   ##
========================================
+ Coverage   62.66%    64%   +1.33%     
========================================
  Files          11     14       +3     
  Lines        1875   1989     +114     
========================================
+ Hits         1175   1273      +98     
- Misses        700    716      +16
Impacted Files Coverage Δ
ptvsd/__main__.py 91.54% <100%> (ø) ⬆️
ptvsd/pydevd_hooks.py 61.29% <61.29%> (ø)
ptvsd/daemon.py 67.22% <67.22%> (ø)
ptvsd/wrapper.py 50.47% <78.57%> (+0.18%) ⬆️
ptvsd/socket.py 90.32% <90.32%> (ø)
ptvsd/ipcjson.py 66.82% <0%> (+1.46%) ⬆️

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 9be9db4...7026e06. Read the comment docs.

@int19h
Copy link
Contributor

int19h commented Apr 3, 2018 •
edited
Loading

I really like this code reorganization, much neater compared to our existing everything-in-wrapper.py! 👍

@DonJayamanne DonJayamanne mentioned this pull request Apr 3, 2018
Closed
DonJayamanne
DonJayamanne suggested changes Apr 3, 2018
View reviewed changes
Copy link
Contributor

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

Running PTVSD fails (error message attached)

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Apr 3, 2018 •
edited
Loading

@ericsnowcurrently
This PR breaks debugging using launch as well as attach.
Looks like we need some better unit tests. #292 is another example where we need unit tests for a similar if not exact scenario. I've attempted to fix the issue without any tests.

screen shot 2018-04-02 at 9 32 45 pm

screen shot 2018-04-02 at 9 37 39 pm

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Apr 3, 2018

I really like this code reorganization, much neater compared to our existing everything-in-wrapper.py! 👍

Agreed.

@ericsnowcurrently
Copy link
Contributor Author

ericsnowcurrently commented Apr 4, 2018

@DonJayamanne This PR should no longer break debugging. Please verify. As to the tests, I agree and am working on it. :)

@ericsnowcurrently ericsnowcurrently changed the title WIP: Fix support for re-attach. Refactor before fixing support for re-attach. Apr 4, 2018
@ericsnowcurrently ericsnowcurrently merged commit 5832fbd into microsoft:master Apr 4, 2018
@ericsnowcurrently ericsnowcurrently deleted the reattach branch April 4, 2018 17:48
@DonJayamanne DonJayamanne mentioned this pull request Apr 7, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@DonJayamanne DonJayamanne DonJayamanne approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ericsnowcurrently @codecov-io @int19h @DonJayamanne