Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable turning off the variable provider #24231

Merged
merged 3 commits into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,12 @@
"experimental"
]
},
"python.REPL.provideVariables": {
"default": true,
"description": "%python.REPL.provideVariables.description%",
"scope": "resource",
"type": "boolean"
},
"python.testing.autoTestDiscoverOnSaveEnabled": {
"default": true,
"description": "%python.testing.autoTestDiscoverOnSaveEnabled.description%",
Expand Down
1 change: 1 addition & 0 deletions package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
"python.pixiToolPath.description": "Path to the pixi executable.",
"python.EnableREPLSmartSend.description": "Toggle Smart Send for the Python REPL. Smart Send enables sending the smallest runnable block of code to the REPL on Shift+Enter and moves the cursor accordingly.",
"python.REPL.sendToNativeREPL.description": "Toggle to send code to Python REPL instead of the terminal on execution. Turning this on will change the behavior for both Smart Send and Run Selection/Line in the Context Menu.",
"python.REPL.provideVariables.description": "Toggle to provide variables for the REPL variable view for the native REPL.",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwebster-99 Do you have suggestion on wording here?
This is for variable viewer for native REPL.

"python.tensorBoard.logDirectory.description": "Set this setting to your preferred TensorBoard log directory to skip log directory prompt when starting TensorBoard.",
"python.tensorBoard.logDirectory.markdownDeprecationMessage": "Tensorboard support has been moved to the extension [Tensorboard extension](https://marketplace.visualstudio.com/items?itemName=ms-toolsai.tensorboard). Instead use the setting `tensorBoard.logDirectory`.",
"python.tensorBoard.logDirectory.deprecationMessage": "Tensorboard support has been moved to the extension Tensorboard extension. Instead use the setting `tensorBoard.logDirectory`.",
Expand Down
17 changes: 15 additions & 2 deletions src/client/repl/variables/variablesProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@ import {
EventEmitter,
Event,
NotebookVariableProvider,
Uri,
} from 'vscode';
import { VariableResultCache } from './variableResultCache';
import { IVariableDescription } from './types';
import { VariableRequester } from './variableRequester';
import { getConfiguration } from '../../common/vscodeApis/workspaceApis';

export class VariablesProvider implements NotebookVariableProvider {
private readonly variableResultCache = new VariableResultCache();
Expand All @@ -36,7 +38,9 @@ export class VariablesProvider implements NotebookVariableProvider {
const notebook = this.getNotebookDocument();
if (notebook) {
this.executionCount += 1;
this._onDidChangeVariables.fire(notebook);
if (isEnabled(notebook.uri)) {
this._onDidChangeVariables.fire(notebook);
}
}
}

Expand All @@ -48,7 +52,12 @@ export class VariablesProvider implements NotebookVariableProvider {
token: CancellationToken,
): AsyncIterable<VariablesResult> {
const notebookDocument = this.getNotebookDocument();
if (token.isCancellationRequested || !notebookDocument || notebookDocument !== notebook) {
if (
!isEnabled(notebook.uri) ||
token.isCancellationRequested ||
!notebookDocument ||
notebookDocument !== notebook
) {
return;
}

Expand Down Expand Up @@ -144,3 +153,7 @@ function getVariableResultCacheKey(uri: string, parent: Variable | undefined, st
}
return `${uri}:${parentKey}`;
}

function isEnabled(resource?: Uri) {
amunger marked this conversation as resolved.
Show resolved Hide resolved
return getConfiguration('python', resource).get('REPL.provideVariables');
}
61 changes: 60 additions & 1 deletion src/test/repl/variableProvider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,29 @@
// Licensed under the MIT License.

import { assert } from 'chai';
import { NotebookDocument, CancellationTokenSource, VariablesResult, Variable, EventEmitter } from 'vscode';
import sinon from 'sinon';
import {
NotebookDocument,
CancellationTokenSource,
VariablesResult,
Variable,
EventEmitter,
ConfigurationScope,
WorkspaceConfiguration,
} from 'vscode';
import * as TypeMoq from 'typemoq';
import { IVariableDescription } from '../../client/repl/variables/types';
import { VariablesProvider } from '../../client/repl/variables/variablesProvider';
import { VariableRequester } from '../../client/repl/variables/variableRequester';
import * as workspaceApis from '../../client/common/vscodeApis/workspaceApis';

suite('ReplVariablesProvider', () => {
let provider: VariablesProvider;
let varRequester: TypeMoq.IMock<VariableRequester>;
let notebook: TypeMoq.IMock<NotebookDocument>;
let getConfigurationStub: sinon.SinonStub;
let configMock: TypeMoq.IMock<WorkspaceConfiguration>;
let enabled: boolean;
const executionEventEmitter = new EventEmitter<void>();
const cancellationToken = new CancellationTokenSource().token;

Expand Down Expand Up @@ -68,9 +81,23 @@ suite('ReplVariablesProvider', () => {
}

setup(() => {
enabled = true;
varRequester = TypeMoq.Mock.ofType<VariableRequester>();
notebook = TypeMoq.Mock.ofType<NotebookDocument>();
provider = new VariablesProvider(varRequester.object, () => notebook.object, executionEventEmitter.event);
configMock = TypeMoq.Mock.ofType<WorkspaceConfiguration>();
configMock.setup((c) => c.get<boolean>('REPL.provideVariables')).returns(() => enabled);
getConfigurationStub = sinon.stub(workspaceApis, 'getConfiguration');
getConfigurationStub.callsFake((section?: string, _scope?: ConfigurationScope | null) => {
if (section === 'python') {
return configMock.object;
}
return undefined;
});
});

teardown(() => {
sinon.restore();
});

test('provideVariables without parent should yield variables', async () => {
Expand All @@ -84,6 +111,38 @@ suite('ReplVariablesProvider', () => {
assert.equal(results[0].variable.expression, 'myObject');
});

test('No variables are returned when variable provider is disabled', async () => {
enabled = false;
setVariablesForParent(undefined, [objectVariable]);

const results = await provideVariables(undefined);

assert.isEmpty(results);
});

test('No change event from provider when disabled', async () => {
enabled = false;
let eventFired = false;
provider.onDidChangeVariables(() => {
eventFired = true;
});

executionEventEmitter.fire();

assert.isFalse(eventFired, 'event should not have fired');
});

test('Variables change event from provider should fire when execution happens', async () => {
let eventFired = false;
provider.onDidChangeVariables(() => {
eventFired = true;
});

executionEventEmitter.fire();

assert.isTrue(eventFired, 'event should have fired');
});

test('provideVariables with a parent should call get children correctly', async () => {
const listVariableItems = [0, 1, 2].map(createListItem);
setVariablesForParent(undefined, [objectVariable]);
Expand Down
Loading