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

Remove controllers from ThirdParty Kernels #10884

Merged
merged 8 commits into from
Jul 21, 2022

Conversation

DonJayamanne
Copy link
Contributor

@DonJayamanne DonJayamanne commented Jul 21, 2022

Part of #10832

  • Split IKernel into 3rd party and regular kernels (regular = kernels used by us)
  • Split IKernelProvider into 3rd party and regular provider

@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2022

Codecov Report

Merging #10884 (0140fbd) into main (64a6e62) will increase coverage by 0%.
The diff coverage is 84%.

❗ Current head 0140fbd differs from pull request most recent head b3d29c8. Consider uploading reports for the commit b3d29c8 to get more accurate results

@@          Coverage Diff           @@
##            main   #10884   +/-   ##
======================================
  Coverage     63%      63%           
======================================
  Files        488      488           
  Lines      33827    33873   +46     
  Branches    5508     5513    +5     
======================================
+ Hits       21442    21514   +72     
+ Misses     10337    10318   -19     
+ Partials    2048     2041    -7     
Impacted Files Coverage Δ
src/interactive-window/interactiveWindow.ts 74% <ø> (ø)
src/kernels/errors/kernelErrorHandler.ts 62% <0%> (+1%) ⬆️
src/kernels/helpers.ts 59% <ø> (ø)
src/kernels/types.ts 100% <ø> (ø)
.../notebooks/controllers/vscodeNotebookController.ts 82% <ø> (+1%) ⬆️
src/notebooks/debugger/debuggingManagerBase.ts 74% <ø> (+5%) ⬆️
src/notebooks/notebookCommandListener.ts 52% <ø> (ø)
src/standalone/api/kernelApi.ts 73% <66%> (+1%) ⬆️
src/kernels/execution/kernelExecution.ts 69% <74%> (-1%) ⬇️
src/notebooks/controllers/kernelConnector.ts 83% <80%> (+<1%) ⬆️
... and 24 more

@DonJayamanne DonJayamanne marked this pull request as ready for review July 21, 2022 18:56
@DonJayamanne DonJayamanne requested a review from a team as a code owner July 21, 2022 18:56
private readonly documentExecutions = new WeakMap<NotebookDocument, CellExecutionQueue>();
private readonly executionFactory: CellExecutionFactory;
private readonly disposables: IDisposable[] = [];
export class BaseKernelExecution<TKernel extends IBaseKernel = IBaseKernel> implements IDisposable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the base class live in a different file? Not sure if that's preferable or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can, sometimes i find it useful to be in one place, else theres a lot of going back and forth..
However Will change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damn it, forgot this, will do in debt week

@DonJayamanne DonJayamanne merged commit 545c0a7 into main Jul 21, 2022
@DonJayamanne DonJayamanne deleted the removeControllerFrom3rdParty branch July 21, 2022 23:17
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.

4 participants