Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
rchiodo merged 2 commits intomicrosoft:mainfromjanosh:new-cell-on-running-last
Oct 27, 2021
Merged

Add setting jupyter.newCellOnRunLast#7995

rchiodo merged 2 commits intomicrosoft:mainfromjanosh:new-cell-on-running-last
Oct 27, 2021

Conversation

janosh
Copy link
Contributor

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. IsrunCurrentCellAndAddBelow() the right function to modify? If so, should we rename it torunCurrentCellAndMaybeAddBelow?

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has anews 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 runningnpm install (if dependencies have changed).

kleinicke reacted with thumbs up emoji
@janoshjanosh requested a review froma team as acode ownerOctober 21, 2021 18:42
@rchiodo
Copy link
Contributor

Thanks@janosh. Looks good to me.

@rchiodo
Copy link
Contributor

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

@IanMatthewHuff
Copy link
Member

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.

@IanMatthewHuffIanMatthewHuff self-requested a reviewOctober 21, 2021 20:05
@janosh
Copy link
ContributorAuthor

janosh commentedOct 21, 2021
edited
Loading

Oh, I didn't know aboutctrl +enter.

@DonJayamanne
Copy link
Contributor

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
Copy link
ContributorAuthor

janosh commentedOct 22, 2021
edited
Loading

@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.

@rchiodo
Copy link
Contributor

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

@rchiodorchiodo reopened thisOct 22, 2021
@codecov-commenter
Copy link

codecov-commenter commentedOct 22, 2021
edited
Loading

Codecov Report

Merging#7995 (a1ee60c) intomain (cd7931d) willdecrease coverage by0%.
The diff coverage is25%.

@@          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 FilesCoverage Δ
src/client/common/types.ts100% <ø> (ø)
src/client/common/utils/localize.ts95% <ø> (ø)
...ient/datascience/editor-integration/codewatcher.ts68% <0%> (-1%)⬇️
src/client/common/configSettings.ts86% <100%> (+<1%)⬆️
...datascience/editor-integration/codelensprovider.ts69% <0%> (-2%)⬇️
...client/datascience/kernel-launcher/kernelDaemon.ts56% <0%> (-2%)⬇️
src/client/datascience/baseJupyterSession.ts71% <0%> (-2%)⬇️
src/client/datascience/jupyter/kernelVariables.ts56% <0%> (-2%)⬇️
...lient/datascience/jupyter/kernels/cellExecution.ts76% <0%> (-2%)⬇️
...datascience/jupyter/liveshare/hostJupyterServer.ts67% <0%> (-1%)⬇️
... and2 more

@DonJayamanne
Copy link
Contributor

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,

Copy link
Contributor

@DonJayamanneDonJayamanne left a 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

@rchiodorchiodo dismissedDonJayamanne’sstale reviewOctober 25, 2021 20:26

Triage agreed to take this

@rchiodo
Copy link
Contributor

@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:

image

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
Copy link
Contributor

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

@janosh
Copy link
ContributorAuthor

@rchiodo Did the rebase. Workflows awaiting approval.

@janosh
Copy link
ContributorAuthor

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
Copy link
Contributor

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:

image

janosh reacted with thumbs up emoji

@rchiodorchiodo merged commit3de609d intomicrosoft:mainOct 27, 2021
@rchiodo
Copy link
Contributor

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

janosh reacted with thumbs up emoji

@rchiodo
Copy link
Contributor

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

Thanks again.

@janosh
Copy link
ContributorAuthor

This setting doesn't actually work, new cells are appended on running the last even when set tofalse. 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
Copy link
Contributor

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:

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

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

@janosh
Copy link
ContributorAuthor

@rchiodo Thanks for taking a look and sorry for getting this wrong. I think thetrue there must stay and instead we need to check inrunMatchingCell ifnewCellOnRunLast istrue in which case we don't do anything in theelse case:

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

@rchiodo
Copy link
Contributor

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)

@janoshjanosh mentioned this pull requestNov 12, 2021
9 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@IanMatthewHuffIanMatthewHuffIanMatthewHuff approved these changes

@DonJayamanneDonJayamanneDonJayamanne approved these changes

@rchiodorchiodorchiodo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@janosh@rchiodo@IanMatthewHuff@DonJayamanne@codecov-commenter

[8]ページ先頭

©2009-2025 Movatter.jp