add run above and run below to native notebooks #4240

DavidKutu · 2020-12-16T02:18:06Z

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 enhancements.
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).

DavidKutu · 2020-12-16T02:21:30Z

src/client/datascience/notebook/notebookEditor.ts

+            commandManager.registerCommand(Commands.NativeNotebookRunCellAndAllBelow, (uri) =>
+                this.runCellAndBelow(uri)
+            )
+        );


In old notebooks, the buttons are enabled only when the user selects a cell. In this case I don't know if we can do that.
We can create the context for it, but there's no event to keep it updated.

IanMatthewHuff · 2020-12-16T17:35:17Z

package.json

@@ -279,6 +279,26 @@
                },
                "enablement": "notebookViewType == jupyter-notebook && jupyter.isnotebooktrusted"
            },
+            {
+                "command": "jupyter.notebookEditor.runallcellsabove",


From what I can see we don't camelCase any other command names. So should keep "notebookeditor" here for consistancy.

IanMatthewHuff · 2020-12-16T17:40:34Z

package.nls.json

@@ -394,6 +394,8 @@
    "DataScience.insertAbove": "Insert cell above",
    "DataScience.addCell": "Add cell",
    "DataScience.runAll": "Run all cells",
+    "DataScience.runAbove": "Run cells above",


We already have some command titles from the old code. Do any of the old strings work here so that we don't need to add new ones?

We never had, the ones on toolbar.tsx for example were returning a default.

There are messages for the command palette:
"jupyter.command.jupyter.runallcellsabove.palette.title": "Run Cells Above Current Cell",
"jupyter.command.jupyter.runcurrentcellandallbelow.palette.title": "Run Current Cell and Below",

Should I use those instead? They're a bit different

Hmmm, maybe just leave as is then. Keeps the native notebook stuff separate.

IanMatthewHuff · 2020-12-16T17:41:06Z

src/client/datascience/constants.ts

@@ -49,6 +49,8 @@ export namespace Commands {
    export const RunAllCells = 'jupyter.runallcells';
    export const RunAllCellsAbove = 'jupyter.runallcellsabove';
    export const RunCellAndAllBelow = 'jupyter.runcellandallbelow';
+    export const NativeNotebookRunAllCellsAbove = 'jupyter.notebookEditor.runallcellsabove';


notebookEditor change to notebookeditor here as well.

IanMatthewHuff · 2020-12-16T17:43:52Z

src/client/datascience/notebook/notebookEditor.ts

+            return editor.selection.uri.toString();
+        }
+
+        return '';


I'd make this function return string or undefined. While '' can work as a "not found value" the undefined make it more clear that you have something valid any string is returned and something not valid if undefined. Even if both empty string and undefined are falsy.

IanMatthewHuff · 2020-12-16T17:44:39Z

src/client/datascience/notebook/notebookEditor.ts

+
+        cells.forEach(async (cell) => {
+            // 2 means code cell
+            if (cell.cellKind === 2) {


You can use the enum here I think:
vscodeNotebookEnums.CellKind.Code

IanMatthewHuff

David. Code looks solid here to me with a few small suggestions. I do think that we should see about adding in a .vscode. test here for this functionality. Hopefully isn't too hard to add, but the functionality here seems to need a test to go along with it.

joyceerhl

Looks good. A vscode test could be helpful here.

codecov-io · 2020-12-17T05:06:51Z

Codecov Report

Merging #4240 (7662e0e) into main (954d4bf) will increase coverage by 2%.
The diff coverage is 20%.

@@          Coverage Diff           @@
##            main   #4240    +/-   ##
======================================
+ Coverage     71%     73%    +2%     
======================================
  Files        384     384            
  Lines      25155   25206    +51     
  Branches    3598    3604     +6     
======================================
+ Hits       17931   18561   +630     
+ Misses      5826    5246   -580     
- Partials    1398    1399     +1

Impacted Files	Coverage Δ
...ient/datascience/interactive-ipynb/nativeEditor.ts	`74% <0%> (-3%)`	⬇️
src/client/datascience/notebook/notebookEditor.ts	`6% <0%> (-1%)`	⬇️
src/client/datascience/types.ts	`100% <ø> (ø)`
...e/interactive-ipynb/nativeEditorCommandListener.ts	`50% <53%> (+11%)`	⬆️
src/client/datascience/constants.ts	`99% <100%> (+<1%)`	⬆️
...e/raw-kernel/liveshare/guestRawNotebookProvider.ts	`52% <0%> (-24%)`	⬇️
...c/client/datascience/variablesView/variableView.ts	`32% <0%> (-18%)`	⬇️
...ce/raw-kernel/liveshare/hostRawNotebookProvider.ts	`80% <0%> (-9%)`	⬇️
...t/common/variables/environmentVariablesProvider.ts	`77% <0%> (-4%)`	⬇️
src/client/datascience/jupyter/jupyterSession.ts	`82% <0%> (-4%)`	⬇️
... and 74 more

IanMatthewHuff · 2020-12-17T21:52:40Z

src/test/datascience/notebook/notebookEditor.vscode.test.ts

+    test('Run cells above', async function () {
+        // This test is skipped because there is no way of selecting a cell in this context
+        // since by default the first cell is selected nothing happens when running all cells above
+        this.skip();


Looks good. I would just file the issue and then put the issue number here in the comment. That way when the test issue is resolved we know where to go to fix it. It's what we usually do for things like this.

nativeEditorCommandListener

add run above and run below to native notebooks

2026c83

DavidKutu requested review from a team and removed request for a team December 16, 2020 02:18

DavidKutu commented Dec 16, 2020

View reviewed changes

IanMatthewHuff reviewed Dec 16, 2020

View reviewed changes

IanMatthewHuff requested changes Dec 16, 2020

View reviewed changes

address some comments

f500f4a

joyceerhl approved these changes Dec 16, 2020

View reviewed changes

fix tests

1c61c8d

add tests

af4ea9b

DavidKutu requested a review from IanMatthewHuff December 17, 2020 21:10

IanMatthewHuff reviewed Dec 17, 2020

View reviewed changes

IanMatthewHuff approved these changes Dec 17, 2020

View reviewed changes

David Kutugata added 2 commits December 17, 2020 15:00

register commands on the

7662e0e

nativeEditorCommandListener

skipt tests

7ea82bb

DavidKutu marked this pull request as ready for review December 18, 2020 00:06

DavidKutu merged commit 6c215b1 into main Dec 18, 2020

DavidKutu deleted the david/runAboveAndBelow branch December 18, 2020 00:35

rebornix mentioned this pull request Dec 21, 2020

Feedback from Jupyter notebook extension early adopters microsoft/vscode#113245

Closed

42 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

add run above and run below to native notebooks #4240

add run above and run below to native notebooks #4240

Uh oh!

DavidKutu commented Dec 16, 2020

Uh oh!

DavidKutu Dec 16, 2020

Uh oh!

IanMatthewHuff Dec 16, 2020

Uh oh!

IanMatthewHuff Dec 16, 2020

Uh oh!

DavidKutu Dec 16, 2020

Uh oh!

IanMatthewHuff Dec 16, 2020

Uh oh!

IanMatthewHuff Dec 16, 2020

Uh oh!

IanMatthewHuff Dec 16, 2020

Uh oh!

IanMatthewHuff Dec 16, 2020

Uh oh!

IanMatthewHuff left a comment

Uh oh!

joyceerhl left a comment

Uh oh!

codecov-io commented Dec 17, 2020 •

edited

Loading

Uh oh!

IanMatthewHuff Dec 17, 2020

Uh oh!

Uh oh!

add run above and run below to native notebooks #4240

add run above and run below to native notebooks #4240

Uh oh!

Conversation

DavidKutu commented Dec 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff left a comment

Choose a reason for hiding this comment

Uh oh!

joyceerhl left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Dec 17, 2020 •

edited

Loading