- Notifications
You must be signed in to change notification settings - Fork332
Add setting jupyter.newCellOnRunLast#7995
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks@janosh. Looks good to me. |
Looks like some of the test files are failing to build. Your new setting isn't in the dummy test configs. |
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 commentedOct 21, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Oh, I didn't know aboutctrl +enter. |
janosh commentedOct 22, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@DonJayamanne I agree there's no need for#7997 but still think this would be nice to have. Even knowing aboutctrl+enter I expect I'll continue to hitshift+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. |
This one seems different to me. I think we should reopen it. |
codecov-commenter commentedOct 22, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ 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
|
Ooops, sorry I didn't mean to close this, I only meant to update it with my comments. Apologies for that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Blocking so we discuss this in triage meeting
@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:
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:
|
You might also just try rebasing against our main branch. Don has fixed a bunch of the tests that were failing. |
…JupyterSettings' but required in type 'IWatchableJupyterSettings'
@rchiodo Did the rebase. Workflows awaiting approval. |
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. |
Oh whoops, there's no news item. I'll add a PR shortly with your fix. |
Added a commit directly with the news item: Thanks again. |
This setting doesn't actually work, new cells are appended on running the last even when set to const{ newCellOnRunLast}=this.configService.getSettings(this.documentManager.activeTextEditor.document.uri); from another place in the code by analogy. |
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:
Instead of passing true for the 'advance' parameter, it should check the setting you added. |
@rchiodo Thanks for taking a look and sorry for getting this wrong. I think the vscode-jupyter/src/client/datascience/editor-integration/codewatcher.ts Lines 1042 to 1048 in796361a
|
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) |
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 torunCurrentCellAndMaybeAddBelow
?Has sufficient logging.package-lock.json
has been regenerated by runningnpm install
(if dependencies have changed).