Skip to content

Fix onInterpreterChange event to only fire on interpreter change #3847

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 3, 2019
Merged

Fix onInterpreterChange event to only fire on interpreter change #3847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6ff509
fix onInterpreterChanged event
IanMatthewHuff Jan 2, 2019
fac340e
add new entry
IanMatthewHuff Jan 2, 2019
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/2 Fixes/3749.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Don't restart the Jupyter server on any settings change. Also don't throw interpreter changed events on unrelated settings changes.
19 changes: 11 additions & 8 deletions src/client/common/configSettings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import * as child_process from 'child_process';
import { EventEmitter } from 'events';
import * as path from 'path';
import {
ConfigurationTarget, DiagnosticSeverity, Disposable, Uri,
ConfigurationChangeEvent, ConfigurationTarget, DiagnosticSeverity, Disposable, Uri,
workspace, WorkspaceConfiguration
} from 'vscode';
import { sendTelemetryEvent } from '../telemetry';
Expand Down Expand Up @@ -348,13 +348,16 @@ export class PythonSettings extends EventEmitter implements IPythonSettings {
}
}
protected initialize(): void {
this.disposables.push(workspace.onDidChangeConfiguration(() => {
const currentConfig = workspace.getConfiguration('python', this.workspaceRoot);
this.update(currentConfig);

// If workspace config changes, then we could have a cascading effect of on change events.
// Let's defer the change notification.
setTimeout(() => this.emit('change'), 1);
this.disposables.push(workspace.onDidChangeConfiguration((event: ConfigurationChangeEvent) => {
if (event.affectsConfiguration('python'))
{
const currentConfig = workspace.getConfiguration('python', this.workspaceRoot);
this.update(currentConfig);

// If workspace config changes, then we could have a cascading effect of on change events.
// Let's defer the change notification.
setTimeout(() => this.emit('change'), 1);
}
}));

const initialConfig = workspace.getConfiguration('python', this.workspaceRoot);
Expand Down
21 changes: 15 additions & 6 deletions src/client/interpreter/interpreterService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,17 @@ export class InterpreterService implements Disposable, IInterpreterService {
private readonly fs: IFileSystem;
private readonly persistentStateFactory: IPersistentStateFactory;
private readonly helper: IInterpreterHelper;
private readonly configService: IConfigurationService;
private readonly didChangeInterpreterEmitter = new EventEmitter<void>();
private pythonPathSetting: string = '';

constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
this.locator = serviceContainer.get<IInterpreterLocatorService>(IInterpreterLocatorService, INTERPRETER_LOCATOR_SERVICE);
this.helper = serviceContainer.get<IInterpreterHelper>(IInterpreterHelper);
this.pythonPathUpdaterService = this.serviceContainer.get<IPythonPathUpdaterServiceManager>(IPythonPathUpdaterServiceManager);
this.fs = this.serviceContainer.get<IFileSystem>(IFileSystem);
this.persistentStateFactory = this.serviceContainer.get<IPersistentStateFactory>(IPersistentStateFactory);
this.configService = this.serviceContainer.get<IConfigurationService>(IConfigurationService);
}

public async refresh(resource?: Uri) {
Expand All @@ -44,8 +47,9 @@ export class InterpreterService implements Disposable, IInterpreterService {
const disposables = this.serviceContainer.get<Disposable[]>(IDisposableRegistry);
const documentManager = this.serviceContainer.get<IDocumentManager>(IDocumentManager);
disposables.push(documentManager.onDidChangeActiveTextEditor((e) => e ? this.refresh(e.document.uri) : undefined));
const configService = this.serviceContainer.get<IConfigurationService>(IConfigurationService);
(configService.getSettings() as PythonSettings).addListener('change', this.onConfigChanged);
const pySettings = this.configService.getSettings() as PythonSettings;
this.pythonPathSetting = pySettings.pythonPath;
pySettings.addListener('change', this.onConfigChanged);
}

public async getInterpreters(resource?: Uri): Promise<PythonInterpreter[]> {
Expand Down Expand Up @@ -238,9 +242,14 @@ export class InterpreterService implements Disposable, IInterpreterService {
}

private onConfigChanged = () => {
this.didChangeInterpreterEmitter.fire();
const interpreterDisplay = this.serviceContainer.get<IInterpreterDisplay>(IInterpreterDisplay);
interpreterDisplay.refresh()
.catch(ex => console.error('Python Extension: display.refresh', ex));
// Check if we actually changed our python path
const pySettings = this.configService.getSettings() as PythonSettings;
if (this.pythonPathSetting !== pySettings.pythonPath) {
this.pythonPathSetting = pySettings.pythonPath;
Copy link

@rchiodo rchiodo Jan 2, 2019

Choose a reason for hiding this comment

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

pythonPath [](start = 48, length = 10)

Is this the only thing that can change for an intrepreter? What if I had two virtualenvs with the same path but different names? (Not sure if that's possible or not)
Copy link
Member Author

@IanMatthewHuff IanMatthewHuff Jan 2, 2019

Choose a reason for hiding this comment

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

@rchiodo Yeah, I was a touch worried that I'm being too naive with this check. I'm not checking in until Don takes a look as I assume that he would know best on that. Might need to check more (venvpath condapath) in addition to the python path. I'll give the venv stuff a look myself now just for kicks.
this.didChangeInterpreterEmitter.fire();
const interpreterDisplay = this.serviceContainer.get<IInterpreterDisplay>(IInterpreterDisplay);
interpreterDisplay.refresh()
.catch(ex => console.error('Python Extension: display.refresh', ex));
}
}
}
22 changes: 12 additions & 10 deletions src/test/interpreters/interpreterService.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import { expect, use } from 'chai';
import * as chaiAsPromised from 'chai-as-promised';
import { EventEmitter } from 'events';
import { Container } from 'inversify';
import * as path from 'path';
import * as TypeMoq from 'typemoq';
Expand All @@ -16,7 +15,7 @@ import { IDocumentManager, IWorkspaceService } from '../../client/common/applica
import { getArchitectureDisplayName } from '../../client/common/platform/registry';
import { IFileSystem } from '../../client/common/platform/types';
import { IPythonExecutionFactory, IPythonExecutionService } from '../../client/common/process/types';
import { IConfigurationService, IDisposableRegistry, IPersistentStateFactory } from '../../client/common/types';
import { IConfigurationService, IDisposableRegistry, IPersistentStateFactory, IPythonSettings } from '../../client/common/types';
import * as EnumEx from '../../client/common/utils/enum';
import { noop } from '../../client/common/utils/misc';
import { Architecture } from '../../client/common/utils/platform';
Expand All @@ -36,6 +35,7 @@ import { InterpreterService } from '../../client/interpreter/interpreterService'
import { IVirtualEnvironmentManager } from '../../client/interpreter/virtualEnvs/types';
import { ServiceContainer } from '../../client/ioc/container';
import { ServiceManager } from '../../client/ioc/serviceManager';
import { PYTHON_PATH } from '../common';

use(chaiAsPromised);

Expand Down Expand Up @@ -69,6 +69,8 @@ suite('Interpreters service', () => {
let persistentStateFactory: TypeMoq.IMock<IPersistentStateFactory>;
let pythonExecutionFactory: TypeMoq.IMock<IPythonExecutionFactory>;
let pythonExecutionService: TypeMoq.IMock<IPythonExecutionService>;
let configService: TypeMoq.IMock<IConfigurationService>;
let pythonSettings: TypeMoq.IMock<IPythonSettings>;
type ConfigValue<T> = { key: string; defaultValue?: T; globalValue?: T; workspaceValue?: T; workspaceFolderValue?: T };

function setupSuite() {
Expand All @@ -88,6 +90,11 @@ suite('Interpreters service', () => {
persistentStateFactory = TypeMoq.Mock.ofType<IPersistentStateFactory>();
pythonExecutionFactory = TypeMoq.Mock.ofType<IPythonExecutionFactory>();
pythonExecutionService = TypeMoq.Mock.ofType<IPythonExecutionService>();
configService = TypeMoq.Mock.ofType<IConfigurationService>();

pythonSettings = TypeMoq.Mock.ofType<IPythonSettings>();
pythonSettings.setup(s => s.pythonPath).returns(() => PYTHON_PATH);
configService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object);

pythonExecutionService.setup((p: any) => p.then).returns(() => undefined);
workspace.setup(x => x.getConfiguration('python', TypeMoq.It.isAny())).returns(() => config.object);
Expand All @@ -113,6 +120,7 @@ suite('Interpreters service', () => {
serviceManager.addSingletonInstance<IPersistentStateFactory>(IPersistentStateFactory, persistentStateFactory.object);
serviceManager.addSingletonInstance<IPythonExecutionFactory>(IPythonExecutionFactory, pythonExecutionFactory.object);
serviceManager.addSingletonInstance<IPythonExecutionService>(IPythonExecutionService, pythonExecutionService.object);
serviceManager.addSingletonInstance<IConfigurationService>(IConfigurationService, configService.object);

pipenvLocator = TypeMoq.Mock.ofType<IInterpreterLocatorService>();
wksLocator = TypeMoq.Mock.ofType<IInterpreterLocatorService>();
Expand Down Expand Up @@ -149,21 +157,18 @@ suite('Interpreters service', () => {
});
});

test('Changes to active document should invoke intrepreter.refresh method', async () => {
test('Changes to active document should invoke interpreter.refresh method', async () => {
const service = new InterpreterService(serviceContainer);
const configService = TypeMoq.Mock.ofType<IConfigurationService>();
const documentManager = TypeMoq.Mock.ofType<IDocumentManager>();

let activeTextEditorChangeHandler: Function | undefined;
documentManager.setup(d => d.onDidChangeActiveTextEditor(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(handler => {
activeTextEditorChangeHandler = handler;
return { dispose: noop };
});
serviceManager.addSingletonInstance(IConfigurationService, configService.object);
serviceManager.addSingletonInstance(IDocumentManager, documentManager.object);

// tslint:disable-next-line:no-any
configService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => new EventEmitter() as any);
service.initialize();
const textEditor = TypeMoq.Mock.ofType<TextEditor>();
const uri = Uri.file(path.join('usr', 'file.py'));
Expand All @@ -175,21 +180,18 @@ suite('Interpreters service', () => {
interpreterDisplay.verify(i => i.refresh(TypeMoq.It.isValue(uri)), TypeMoq.Times.once());
});

test('If there is no active document then intrepreter.refresh should not be invoked', async () => {
test('If there is no active document then interpreter.refresh should not be invoked', async () => {
const service = new InterpreterService(serviceContainer);
const configService = TypeMoq.Mock.ofType<IConfigurationService>();
const documentManager = TypeMoq.Mock.ofType<IDocumentManager>();

let activeTextEditorChangeHandler: Function | undefined;
documentManager.setup(d => d.onDidChangeActiveTextEditor(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(handler => {
activeTextEditorChangeHandler = handler;
return { dispose: noop };
});
serviceManager.addSingletonInstance(IConfigurationService, configService.object);
serviceManager.addSingletonInstance(IDocumentManager, documentManager.object);

// tslint:disable-next-line:no-any
configService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => new EventEmitter() as any);
service.initialize();
activeTextEditorChangeHandler!();

Expand Down