Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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
38 changes: 37 additions & 1 deletion src/common/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@

import * as fs from 'fs-extra';
import * as path from 'path';
import { ConfigurationScope, env, LogLevel, Uri, WorkspaceFolder } from 'vscode';
import { ConfigurationScope, env, LogLevel, Uri, WorkspaceFolder, Disposable, RelativePattern,FileSystemWatcher, workspace } from 'vscode';
import { Trace, TraceValues } from 'vscode-jsonrpc/node';
import { getConfiguration, getWorkspaceFolders, isVirtualWorkspace } from './vscodeapi';
import { DocumentSelector } from 'vscode-languageclient';
import { traceLog, traceInfo } from './logging';

function logLevelToTrace(logLevel: LogLevel): Trace {
switch (logLevel) {
Expand Down Expand Up @@ -82,3 +83,38 @@ export function getInterpreterFromSetting(namespace: string, scope?: Configurati
const config = getConfiguration(namespace, scope);
return config.get<string[]>('interpreter');
}

export function createConfigFileWatcher(onConfigChanged: () => Promise<void>): Disposable {
const watchers: FileSystemWatcher[] = [];
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be just disposables of type Disposable[]

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your review.
I've made the changes.


const homeDir = process.env.HOME || process.env.USERPROFILE;
if (homeDir) {
watchConfigFile(path.join(homeDir, '.black'));
}

function watchConfigFile(filePath: string): void {
if (fs.existsSync(filePath)) {
Copy link
Member

Choose a reason for hiding this comment

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

Use async versions for FS apis.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your review.
I've made the changes.

const pattern = new RelativePattern(
path.dirname(filePath),
path.basename(filePath)
);

const watcher = workspace.createFileSystemWatcher(pattern);

watcher.onDidChange(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

onDidChange returns a Disposable so it should be added to disposables

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your review.
I've made the changes.

traceInfo(`Config file changed: ${filePath}`);
await onConfigChanged();
});

watchers.push(watcher);
}
}

return {
dispose: () => {
for (const watcher of watchers) {
watcher.dispose();
}
}
};
Copy link
Member

Choose a reason for hiding this comment

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

This should just be:

return {
    dispose: () => {disposables.forEach((d) => d.dispose())};
}

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your review.
I've made the changes.

}
10 changes: 9 additions & 1 deletion src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@ import {
logLegacySettings,
} from './common/settings';
import { loadServerDefaults } from './common/setup';
import { getInterpreterFromSetting, getProjectRoot } from './common/utilities';
import { getInterpreterFromSetting, getProjectRoot, createConfigFileWatcher } from './common/utilities';
import { createOutputChannel, onDidChangeConfiguration, registerCommand } from './common/vscodeapi';
import { registerEmptyFormatter } from './common/nullFormatter';
import { registerLanguageStatusItem, updateStatus } from './common/status';
import { LS_SERVER_RESTART_DELAY, PYTHON_VERSION } from './common/constants';

let lsClient: LanguageClient | undefined;
let configWatcherDisposable: vscode.Disposable | undefined;

export async function activate(context: vscode.ExtensionContext): Promise<void> {
// This is required to get server name and module. This should be
// the first thing that we do in this extension.
Expand Down Expand Up @@ -69,7 +71,10 @@ export async function activate(context: vscode.ExtensionContext): Promise<void>
}
};

configWatcherDisposable = createConfigFileWatcher(runServer);

context.subscriptions.push(
configWatcherDisposable,
Copy link
Member

Choose a reason for hiding this comment

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

You can call create config watcher here directly. Once you make it async be sure to add await.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your review.
I've modified the code to directly call createConfigFileWatcher in the subscriptions.push() method.

onDidChangePythonInterpreter(async () => {
await runServer();
}),
Expand Down Expand Up @@ -116,6 +121,9 @@ export async function activate(context: vscode.ExtensionContext): Promise<void>
}

export async function deactivate(): Promise<void> {
if (configWatcherDisposable) {
Copy link
Member

Choose a reason for hiding this comment

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

If you put it in context.subscriptions there is no need to explicitly dispose

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your review.
I've removed the explicit dispose()from the deactivate function.

configWatcherDisposable.dispose();
}
if (lsClient) {
await lsClient.stop();
}
Expand Down