Draft for gather extensibility point #13672

DavidKutu · 2020-08-29T00:08:26Z

For #13306

Not final, but would like your review.

codecov-commenter · 2020-08-29T00:30:50Z

Codecov Report

Merging #13672 into main will decrease coverage by 0.02%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##             main   #13672      +/-   ##
==========================================
- Coverage   59.86%   59.84%   -0.03%     
==========================================
  Files         693      693              
  Lines       38351    38355       +4     
  Branches     5516     5516              
==========================================
- Hits        22960    22952       -8     
- Misses      14211    14220       +9     
- Partials     1180     1183       +3

Impacted Files	Coverage Δ
...ient/datascience/interactive-ipynb/nativeEditor.ts	`7.78% <0.00%> (-0.08%)`	⬇️
...atascience/notebookStorage/nativeEditorProvider.ts	`17.26% <ø> (ø)`
src/client/datascience/types.ts	`100.00% <100.00%> (ø)`
src/client/datascience/crossProcessLock.ts	`79.41% <0.00%> (-11.77%)`	⬇️
src/client/common/utils/platform.ts	`60.00% <0.00%> (-8.00%)`	⬇️
src/client/linters/pydocstyle.ts	`86.66% <0.00%> (-2.23%)`	⬇️
src/client/datascience/debugLocationTracker.ts	`76.56% <0.00%> (-1.57%)`	⬇️
src/client/common/process/proc.ts	`14.49% <0.00%> (-0.73%)`	⬇️

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 ed3f29f...2833cb4. Read the comment docs.

DonJayamanne · 2020-08-31T17:12:42Z

src/client/datascience/types.ts

+
+export const IJupyterExecutionLoggerRegistration = Symbol('IJupyterExecutionLoggerRegistration');
+
+export interface IJupyterExecutionLoggerRegistration {


Based on our last discussion, exposing an event from the extension would be better.
This way you won't need to inject logger classes, manage it and the like.
Just fire events & the API is much simpler and smaller (both ends need to expose other types).
Aldo, we discussed that it would be better to not expose ICell outside the extension, expose only what you need index of cell, source of cell and anything else required

rchiodo · 2020-09-01T22:58:47Z

src/client/datascience/notebook/notebookEditor.ts

        this._executed.fire(this);
-        this.executedCode.fire(code);
+        this.executedCode.fire(cell.document.getText());
+        const cellMessage: IPublicCellInfo = {


IPublicCellInfo sounds strange to me. ICellInfo seems appropriate?

Or maybe ICellExecutionInfo?

rchiodo · 2020-09-01T23:00:06Z

src/client/datascience/notebook/notebookEditor.ts

@@ -230,6 +245,7 @@ export class NotebookEditor implements INotebookEditor {
            this.document.metadata.cellRunnable = false;
            this.document.metadata.runnable = false;
            await kernel.restart();
+            this.kernelChange.fire('kernelRestarted');


Two events seems better than the ambiguous 'string' result.

onExecute<IExecutionInfo> onKernelEvent<string>

?

Or maybe

onKernelExecute<IExecutionInfo> onKernelEvent<string>

Do you mean 3 events?

cell executed

notebook opened

kernel restarted

Or maybe it can return CellInfo or an enum

Sorry didn't notice the other use of the string param.

3 events sounds better. Overloading the event to be something like 'stuffChanged' doesn't seem very cohesive to me.

DavidKutu · 2020-09-01T23:39:59Z

src/client/datascience/notebook/notebookEditor.ts

+            source: cell.document.getText(),
+            executionCount: cell.metadata.executionOrder!,
+            executionEventId: uuid(),
+            hasError: cell.metadata.statusMessage! === 'error'


This is not working right, both executionOrder and statusMessage are always undefined and there is no persistent cell ID

Just wondering, are you specifically trying to keep this trimmed to just what you need? Just wasn't sure why not just pass the statusMessage then check in gather to see if it's 'error' or not.

yeah, I followed Don's comment of sending the whole cell

DonJayamanne · 2020-09-02T00:38:31Z

src/client/datascience/types.ts

+    id: string;
+    source: string;
+    executionCount: number;
+    executionEventId: string;


Why do we need this? Seems pointless if it is just generated and not stored anywhere
Why not expose the cell as is? After all this is a VSCode object.

DonJayamanne · 2020-09-02T00:44:29Z

src/client/datascience/interactive-ipynb/nativeEditor.ts

+        return new EventEmitter<ICellExecutionInfo>().event;
+    }
+    public get onKernelRestart(): Event<void> {
+        return new EventEmitter<void>().event;


Better to create a separate class, as a singleton and excise those events there. We want to try to get rid of the old notebook editor interface

E.g. Notebook extensibility.
Add the events here and the methods in here, this way we have a specific class for such extensibility.

DonJayamanne · 2020-09-02T00:46:49Z

src/client/api.ts

@@ -127,7 +137,12 @@ export function buildApi(
                const pythonPath = configurationService.getSettings(resource).pythonPath;
                // If pythonPath equals an empty string, no interpreter is set.
                return { execCommand: pythonPath === '' ? undefined : [pythonPath] };
-            }
+            },
+            onNotebookOpened: notebookEditor ? notebookEditor.onNotebookOpened : new EventEmitter<void>().event,


You can get this from VSCode API, we don't need to expose this event from here, that's one less thing to expose from our end

I can't find it.

Thats how we have it working in the python extension. onDidOpenNotebookDocument

DonJayamanne · 2020-09-02T00:48:05Z

src/client/datascience/notebook/notebookEditor.ts

+            executionEventId: uuid(),
+            hasError: cell.metadata.statusMessage! === 'error'
+        };
+        this.kernelExecute.fire(cellInfo);


Why not fire(cell)
As VSCode cell is a VSCode object have you don't need to expose other interfaces from the python extension

- part 1

and made it a singleton

DonJayamanne · 2020-09-04T18:18:38Z

src/client/api.ts

@@ -51,6 +52,9 @@ export interface IExtensionApi {
         * An event that is emitted when execution details (for a resource) change. For instance, when interpreter configuration changes.
         */
        readonly onDidChangeExecutionDetails: Event<Uri | undefined>;
+        readonly onNotebookOpened: Event<void>;


You can get this from the VS Code API.

where?

Edit: nevermind, I saw your other comment

post execute

DonJayamanne · 2020-09-04T19:36:49Z

src/client/api.ts

@@ -99,6 +102,7 @@ export function buildApi(
 ): IExtensionApi {


This interface has not been updated.

Do you mean updated to PostExecute?

DonJayamanne · 2020-09-04T19:37:13Z

src/client/api.ts

-            }
+            },
+            onKernelExecute: notebookExtensibility.onKernelPostExecute,
+            onKernelRestart: notebookExtensibility.onKernelRestart


Shouldn't these go under dataScience

DonJayamanne

We need to update the API interface & move the events under the DataScience

sonarcloud · 2020-09-11T03:43:28Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

draft for gather extensibility point

ed49a5e

DavidKutu requested review from DonJayamanne, greazer, IanMatthewHuff, joyceerhl and rchiodo August 29, 2020 00:11

fix compiler errors

aa40882

DonJayamanne reviewed Aug 31, 2020

View reviewed changes

unknown added 5 commits August 31, 2020 12:05

add event on extension api

fc021ce

added an event to the api

dff1cf7

oops

08b0191

remove old implementation

f7d60b3

fix activation errors

e2d49cb

DavidKutu requested a review from DonJayamanne September 1, 2020 22:50

Merge branch 'master' into david/gatherExtensibilityPoint

852e132

rchiodo reviewed Sep 1, 2020

View reviewed changes

use 3 events instead of 1

c9cb0be

DavidKutu requested a review from rchiodo September 1, 2020 23:26

rchiodo approved these changes Sep 1, 2020

View reviewed changes

DavidKutu commented Sep 1, 2020

View reviewed changes

DonJayamanne requested changes Sep 2, 2020

View reviewed changes

DonJayamanne reviewed Sep 2, 2020

View reviewed changes

David Kutugata added 2 commits September 2, 2020 11:46

create a class for the execution logger events

4dae5a4

- part 1

put the logger into a class

2734f79

DavidKutu requested a review from DonJayamanne September 2, 2020 21:09

rename logger to notebook extensibility

faf18be

and made it a singleton

David Kutugata added 2 commits September 2, 2020 15:57

oops

158c6b6

Merge branch 'master' into david/gatherExtensibilityPoint

7ace45c

microsoft-github-updates bot changed the base branch from master to main September 3, 2020 21:06

fix functional tests

3e82629

DonJayamanne requested changes Sep 4, 2020

View reviewed changes

David Kutugata added 2 commits September 4, 2020 11:37

fix more functional tests

27a3d2e

remove onNotebookOpened, especify event name as

605574a

post execute

DavidKutu requested a review from DonJayamanne September 4, 2020 19:11

DonJayamanne reviewed Sep 4, 2020

View reviewed changes

DonJayamanne requested changes Sep 4, 2020

View reviewed changes

moved to the datascience part of the API

2c8bd07

DavidKutu requested a review from DonJayamanne September 4, 2020 20:05

DonJayamanne approved these changes Sep 4, 2020

View reviewed changes

David Kutugata added 2 commits September 10, 2020 15:22

Merge branch 'main' into david/gatherExtensibilityPoint

99d1932

add news

6ef42fc

DavidKutu marked this pull request as ready for review September 10, 2020 23:46

Merge branch 'main' into david/gatherExtensibilityPoint

2833cb4

rchiodo merged commit c9f2d9c into main Sep 11, 2020

rchiodo deleted the david/gatherExtensibilityPoint branch September 11, 2020 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft for gather extensibility point #13672

Draft for gather extensibility point #13672

DavidKutu commented Aug 29, 2020 •

edited

codecov-commenter commented Aug 29, 2020 •

edited

DonJayamanne Aug 31, 2020

rchiodo Sep 1, 2020

rchiodo Sep 1, 2020

rchiodo Sep 1, 2020

rchiodo Sep 1, 2020

rchiodo Sep 1, 2020

DavidKutu Sep 1, 2020

DavidKutu Sep 1, 2020

rchiodo Sep 1, 2020

DavidKutu Sep 1, 2020

IanMatthewHuff Sep 2, 2020

DavidKutu Sep 2, 2020

DonJayamanne Sep 2, 2020

DonJayamanne Sep 2, 2020

DonJayamanne Sep 2, 2020 •

edited

DonJayamanne Sep 2, 2020

DavidKutu Sep 2, 2020

DonJayamanne Sep 4, 2020

DonJayamanne Sep 2, 2020

DonJayamanne Sep 4, 2020

DavidKutu Sep 4, 2020 •

edited

DonJayamanne Sep 4, 2020

DavidKutu Sep 4, 2020 •

edited

DonJayamanne Sep 4, 2020

DonJayamanne left a comment

sonarcloud bot commented Sep 11, 2020


		export const IJupyterExecutionLoggerRegistration = Symbol('IJupyterExecutionLoggerRegistration');

		export interface IJupyterExecutionLoggerRegistration {

		@@ -99,6 +102,7 @@ export function buildApi(
		): IExtensionApi {

Draft for gather extensibility point #13672

Draft for gather extensibility point #13672

Conversation

DavidKutu commented Aug 29, 2020 • edited

codecov-commenter commented Aug 29, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne Sep 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidKutu Sep 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidKutu Sep 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 11, 2020

DavidKutu commented Aug 29, 2020 •

edited

codecov-commenter commented Aug 29, 2020 •

edited

DonJayamanne Sep 2, 2020 •

edited

DavidKutu Sep 4, 2020 •

edited

DavidKutu Sep 4, 2020 •

edited