Skip to content
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

Rebornix/move debugger out of kernels #10870

Merged

Conversation

rebornix
Copy link
Member

@rebornix rebornix commented Jul 20, 2022

While reading through #10855, it seems that kernels/debugger is directly used in notebooks and interactive window and no other sub components in kernels actually rely on it (only exception is kernels/variables/debugVariable but that is still related to debugging).

This PR is an attempt to move debugging feature code into notebooks and it proved our hypothesis that debugging is a standalone feature, moving it out doesn't modify any other file in kernels. This also opens the door of reviewing debugging code in notebooks and interactive-window and see if they can be unified.

  • 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 feature-requests.
  • 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).

@rebornix rebornix requested a review from a team as a code owner July 20, 2022 05:38
@rebornix rebornix force-pushed the rebornix/move-debugger-out-of-kernels branch from dc5137b to 3667f4e Compare July 20, 2022 06:34
@codecov-commenter
Copy link

Codecov Report

Merging #10870 (3667f4e) into main (c0fe4cc) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##            main   #10870   +/-   ##
======================================
  Coverage     52%      52%           
======================================
  Files        486      486           
  Lines      33787    33787           
  Branches    5501     5501           
======================================
+ Hits       17782    17795   +13     
+ Misses     14286    14275   -11     
+ Partials    1719     1717    -2     
Impacted Files Coverage Δ
src/interactive-window/debugger/helper.ts 33% <ø> (ø)
src/interactive-window/types.ts 100% <ø> (ø)
src/kernels/serviceRegistry.node.ts 94% <ø> (-1%) ⬇️
src/notebooks/debugger/constants.ts 100% <ø> (ø)
src/notebooks/debugger/debugLocationTracker.ts 73% <ø> (ø)
.../notebooks/debugger/debugLocationTrackerFactory.ts 50% <ø> (ø)
src/notebooks/debugger/debuggingTypes.ts 100% <ø> (ø)
src/notebooks/debugger/jupyterDebugService.node.ts 17% <ø> (ø)
...-window/debugger/interactiveWindowDebugger.node.ts 11% <100%> (ø)
...ve-window/debugger/jupyter/debugCellControllers.ts 23% <100%> (ø)
... and 19 more

@rebornix rebornix merged commit c68a256 into microsoft:main Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants