Add setting jupyter.newCellOnRunLast

[janosh]

For #7966.

Went ahead and took a swing at adding a setting to disable appending a new empty cell to IW files when running the last cell. Not sure how successfully tbh. Is runCurrentCellAndAddBelow() the right function to modify? If so, should we rename it to runCurrentCellAndMaybeAddBelow?

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

[rchiodo]

Thanks @janosh. Looks good to me.

[rchiodo]

Looks like some of the test files are failing to build. Your new setting isn't in the dummy test configs.

[IanMatthewHuff]

I think that I'm fine with this setting (since I do add that cell on accident myself from time to time), but ctrl-enter does exist for this scenario already which is what I usually use. Feels a little funny to have a setting just to basically map shift-enter => ctrl-enter in a specific scenario. I think I like it though, not like an extra setting hurts much.

[janosh]

Oh, I didn't know about ctrl + enter.

[DonJayamanne]

Thanks @janosh for the PR.

However I'm not yet convinced we should be adding these two settings.
Would like to discuss this with the rest of the team along with #7995 once again.

[janosh]

@DonJayamanne I agree there's no need for #7997 but still think this would be nice to have. Even knowing about ctrl+enter I expect I'll continue to hit shift+enter occasionally out of habit. This then adds new empty cells to files I didn't not mean to modify which in turn causes commit hooks to complain, requiring me to revert the offending files. So I find the current cell-adding behavior a bit of a hassle and would like to disable it completely.

[rchiodo]

This one seems different to me. I think we should reopen it.

[codecov-commenter]

Codecov Report

Merging #7995 (a1ee60c) into main (cd7931d) will decrease coverage by 0%.
The diff coverage is 25%.

@@          Coverage Diff          @@
##            main   #7995   +/-   ##
=====================================
- Coverage     70%     70%   -1%     
=====================================
  Files        367     367           
  Lines      22613   22616    +3     
  Branches    3429    3430    +1     
=====================================
- Hits       16041   16032    -9     
- Misses      5189    5193    +4     
- Partials    1383    1391    +8     

Impacted Files	Coverage Δ
src/client/common/types.ts	100% <ø> (ø)
src/client/common/utils/localize.ts	95% <ø> (ø)
...ient/datascience/editor-integration/codewatcher.ts	68% <0%> (-1%)	⬇️
src/client/common/configSettings.ts	86% <100%> (+<1%)	⬆️
...datascience/editor-integration/codelensprovider.ts	69% <0%> (-2%)	⬇️
...client/datascience/kernel-launcher/kernelDaemon.ts	56% <0%> (-2%)	⬇️
src/client/datascience/baseJupyterSession.ts	71% <0%> (-2%)	⬇️
src/client/datascience/jupyter/kernelVariables.ts	56% <0%> (-2%)	⬇️
...lient/datascience/jupyter/kernels/cellExecution.ts	76% <0%> (-2%)	⬇️
...datascience/jupyter/liveshare/hostJupyterServer.ts	67% <0%> (-1%)	⬇️
... and 2 more

[DonJayamanne]

Ooops, sorry I didn't mean to close this, I only meant to update it with my comments. Apologies for that.
Lets discuss in the triage meeting. If anything I'd like the issue to be left open and wait for community upvotes,

[DonJayamanne]

Blocking so we discuss this in triage meeting

Triage agreed to take this

[rchiodo]

@janosh we agreed to take this one, but I think it's breaking some of the tests. Specifically this one might be related to your change:

  Leading whitespace not suppressed:1 | Output does not contain provided text '	ho 	ho 	ho ' for Cell 1, it is <No cell outputs>

Can you try that locally?

You can do that inside VS code (if you run with insiders), by running this launch config:

And changing the launch config to use a grep:

        {
            "name": "VS Code Tests (Jupyter+Python Extension installed, *.vscode.test.ts)",
            "type": "extensionHost",
            "request": "launch",
            "runtimeExecutable": "${execPath}",
            "args": [
                "${workspaceFolder:vscode-jupyter}/src/test/datascience",
                "--enable-proposed-api",
                "--extensionDevelopmentPath=${workspaceFolder}",
                "--extensionTestsPath=${workspaceFolder}/out/test"
            ],
            "env": {
--->           "VSC_JUPYTER_CI_TEST_GREP": "Check diagnostics with and without an import", // Modify this to run a subset of the tests
                "VSC_JUPYTER_CI_TEST_INVERT_GREP": "", // Initialize this to invert the grep (exclude tests with value defined in grep).
                "CI_PYTHON_PATH": "<PythonPath>", // Update with path to real python interpereter used for testing.
                "VSC_FORCE_REAL_JUPYTER": "true", // Enalbe tests that require Jupyter.
                "VSC_JUPYTER_CI_RUN_NON_PYTHON_NB_TEST": "", // Initialize this to run tests again Julia & other kernels.
                "VSC_JUPYTER_RUN_NB_TEST": "1", // Initialize this to run notebook tests (must be using VSC Insiders).
                "VSC_JUPYTER_LOAD_EXPERIMENTS_FROM_FILE": "true",
                "TEST_FILES_SUFFIX": "vscode.test",
                "VSC_JUPYTER_EXPOSE_SVC": "1"
            },
            "stopOnEntry": false,
            "sourceMaps": true,
            "outFiles": ["${workspaceFolder}/out/**/*.js", "!${workspaceFolder}/**/node_modules**/*"],
            "preLaunchTask": "Compile",
            "skipFiles": ["<node_internals>/**"],
            "presentation": {
                "group": "2_tests",
                "order": 5
            }
        },

[rchiodo]

You might also just try rebasing against our main branch. Don has fixed a bunch of the tests that were failing.

[janosh]

@rchiodo Did the rebase. Workflows awaiting approval.

[janosh]

Sadly tests are still failing. They're also difficult to make sense of. "Run tests with VSCode & Jupyter" is the one affected and it has a mile-long output. Not sure what the actual error is.

[rchiodo]

Close enough. I think those two tests are flakey.

Thanks so much for the PR. I'm going to submit.

If you happen to help us again, and tests fail, you can look in the publish test results item to see which test failed:

[rchiodo]

Oh whoops, there's no news item. I'll add a PR shortly with your fix.

[rchiodo]

Added a commit directly with the news item:
bd2d62a#diff-15ec3520a6c835df7ee611f3fa1dd1e0117da936621083ce38df337269389426

Thanks again.

[janosh]

This setting doesn't actually work, new cells are appended on running the last even when set to false. Probably should have added a test as well. Am I fetching the setting the right way? I just copied

const { newCellOnRunLast } = this.configService.getSettings(this.documentManager.activeTextEditor.document.uri);

from another place in the code by analogy.

[rchiodo]

Yeah that looks correct to me.

I debugged it a bit. I think the check is in the wrong spot.

I believe it should be here:

vscode-jupyter/src/client/datascience/editor-integration/codewatcher.ts

Line 343 in 796361a

return this.runMatchingCell(this.documentManager.activeTextEditor.selection, true);

Instead of passing true for the 'advance' parameter, it should check the setting you added.

[janosh]

@rchiodo Thanks for taking a look and sorry for getting this wrong. I think the true there must stay and instead we need to check in runMatchingCell if newCellOnRunLast is true in which case we don't do anything in the else case:

vscode-jupyter/src/client/datascience/editor-integration/codewatcher.ts

Lines 1042 to 1048 in 796361a

	} else {
	// insert new cell at bottom after current
	const editor = this.documentManager.activeTextEditor;
	if (editor) {
	this.insertCell(editor, currentRunCellLens.range.end.line + 1);
	}
	}

[rchiodo]

Yeah that seems better. I was thinking we only wanted to apply the setting on the SHIFT+ENTER case, but we should apply it on the 'runcell' code lens too (which I believe is the other case that has advance == true)