Skip to content

🧹 Refactor classes that do not need a language client to not inherit from IFeature #2685

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 8 commits into from
Jul 23, 2020
Merged

🧹 Refactor classes that do not need a language client to not inherit from IFeature #2685

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4afcb90
Refactor classes that do not need a language client to not inherit fr…
May 9, 2020
95a7c29
Merge branch 'master' of https://github.com/PowerShell/vscode-powersh…
Jun 7, 2020
57b714b
Use base class LanguageClientConsumer
Jun 8, 2020
a50af97
Remove IFeature by making languageClientConsumer abstract
Jun 10, 2020
c8eedbb
Provide base implementation for setLanguageClient to remove repeated …
Jun 10, 2020
447c184
Merge branch 'master' of https://github.com/PowerShell/vscode-powersh…
Jul 23, 2020
1104820
Adapt ExternalApi.ts and PowerShellNotebooks.ts
Jul 23, 2020
5b9b99d
newline
TylerLeonhardt Jul 23, 2020
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
9 changes: 1 addition & 8 deletions src/features/CodeActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,12 @@
*--------------------------------------------------------*/

import vscode = require("vscode");
import { LanguageClient } from "vscode-languageclient";
import Window = vscode.window;
import { IFeature } from "../feature";
import { ILogger } from "../logging";

export class CodeActionsFeature implements IFeature {
export class CodeActionsFeature implements vscode.Disposable {
private applyEditsCommand: vscode.Disposable;
private showDocumentationCommand: vscode.Disposable;
private languageClient: LanguageClient;

constructor(private log: ILogger) {
this.applyEditsCommand = vscode.commands.registerCommand("PowerShell.ApplyCodeActionEdits", (edit: any) => {
Expand All @@ -37,10 +34,6 @@ export class CodeActionsFeature implements IFeature {
this.showDocumentationCommand.dispose();
}

public setLanguageClient(languageclient: LanguageClient) {
this.languageClient = languageclient;
}

public showRuleDocumentation(ruleId: string) {
const pssaDocBaseURL = "https://github.com/PowerShell/PSScriptAnalyzer/blob/master/RuleDocumentation";

Expand Down
3 changes: 0 additions & 3 deletions src/features/CustomViews.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { IFeature } from "../feature";
export class CustomViewsFeature implements IFeature {

private commands: vscode.Disposable[] = [];
private languageClient: LanguageClient;
private contentProvider: PowerShellContentProvider;

constructor() {
Expand Down Expand Up @@ -65,8 +64,6 @@ export class CustomViewsFeature implements IFeature {
args.id,
args.appendedHtmlBodyContent);
});

this.languageClient = languageClient;
}
}

Expand Down
7 changes: 1 addition & 6 deletions src/features/DebugSession.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,10 +325,9 @@ export class DebugSessionFeature implements IFeature, DebugConfigurationProvider
}
}

export class SpecifyScriptArgsFeature implements IFeature {
export class SpecifyScriptArgsFeature implements vscode.Disposable {

private command: vscode.Disposable;
private languageClient: LanguageClient;
private context: vscode.ExtensionContext;

constructor(context: vscode.ExtensionContext) {
Expand All @@ -340,10 +339,6 @@ export class SpecifyScriptArgsFeature implements IFeature {
});
}

public setLanguageClient(languageclient: LanguageClient) {
this.languageClient = languageclient;
}

public dispose() {
this.command.dispose();
}
Expand Down
8 changes: 1 addition & 7 deletions src/features/Examples.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,8 @@

import path = require("path");
import vscode = require("vscode");
import { LanguageClient } from "vscode-languageclient";
import { IFeature } from "../feature";

export class ExamplesFeature implements IFeature {
export class ExamplesFeature implements vscode.Disposable {
private command: vscode.Disposable;
private examplesPath: string;

Expand All @@ -21,10 +19,6 @@ export class ExamplesFeature implements IFeature {
});
}

public setLanguageClient(languageclient: LanguageClient) {
// Eliminate tslint warning
}

public dispose() {
this.command.dispose();
}
Expand Down
7 changes: 1 addition & 6 deletions src/features/GenerateBugReport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import cp = require("child_process");
import os = require("os");
import vscode = require("vscode");
import { IFeature, LanguageClient } from "../feature";
import { SessionManager } from "../session";
import Settings = require("../settings");

Expand All @@ -27,7 +26,7 @@ const extensions =
return 0;
});

export class GenerateBugReportFeature implements IFeature {
export class GenerateBugReportFeature implements vscode.Disposable {

private command: vscode.Disposable;
private powerShellProcess: cp.ChildProcess;
Expand Down Expand Up @@ -83,10 +82,6 @@ ${this.generateExtensionTable(extensions)}
this.command.dispose();
}

public setLanguageClient(languageclient: LanguageClient) {
// Eliminate tslint warning.
}

private generateExtensionTable(installedExtensions): string {
if (!installedExtensions.length) {
return "none";
Expand Down
9 changes: 1 addition & 8 deletions src/features/ISECompatibility.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
* Copyright (C) Microsoft Corporation. All rights reserved.
*--------------------------------------------------------*/
import * as vscode from "vscode";
import { LanguageClient } from "vscode-languageclient";
import { IFeature } from "../feature";
import * as Settings from "../settings";

interface ISetting {
Expand All @@ -15,7 +13,7 @@ interface ISetting {
/**
* A feature to implement commands to make code like the ISE and reset the settings.
*/
export class ISECompatibilityFeature implements IFeature {
export class ISECompatibilityFeature implements vscode.Disposable {
// Marking settings as public so we can use it within the tests without needing to duplicate the list of settings.
public static settings: ISetting[] = [
{ path: "workbench.activityBar", name: "visible", value: false },
Expand All @@ -27,7 +25,6 @@ export class ISECompatibilityFeature implements IFeature {
];
private iseCommandRegistration: vscode.Disposable;
private defaultCommandRegistration: vscode.Disposable;
private languageClient: LanguageClient;

constructor() {
this.iseCommandRegistration = vscode.commands.registerCommand(
Expand All @@ -41,10 +38,6 @@ export class ISECompatibilityFeature implements IFeature {
this.defaultCommandRegistration.dispose();
}

public setLanguageClient(languageclient: LanguageClient) {
this.languageClient = languageclient;
}

private async EnableISEMode() {
for (const iseSetting of ISECompatibilityFeature.settings) {
await vscode.workspace.getConfiguration(iseSetting.path).update(iseSetting.name, iseSetting.value, true);
Expand Down
7 changes: 1 addition & 6 deletions src/features/OpenInISE.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@

import ChildProcess = require("child_process");
import vscode = require("vscode");
import { IFeature, LanguageClient } from "../feature";

export class OpenInISEFeature implements IFeature {
export class OpenInISEFeature implements vscode.Disposable {
private command: vscode.Disposable;

constructor() {
Expand All @@ -33,8 +32,4 @@ export class OpenInISEFeature implements IFeature {
public dispose() {
this.command.dispose();
}

public setLanguageClient(languageClient: LanguageClient) {
// Not needed for this feature.
}
}
8 changes: 1 addition & 7 deletions src/features/PesterTests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

import * as path from "path";
import vscode = require("vscode");
import { IFeature, LanguageClient } from "../feature";
import { SessionManager } from "../session";
import Settings = require("../settings");
import utils = require("../utils");
Expand All @@ -14,10 +13,9 @@ enum LaunchType {
Run,
}

export class PesterTestsFeature implements IFeature {
export class PesterTestsFeature implements vscode.Disposable {

private command: vscode.Disposable;
private languageClient: LanguageClient;
private invokePesterStubScriptPath: string;

constructor(private sessionManager: SessionManager) {
Expand Down Expand Up @@ -47,10 +45,6 @@ export class PesterTestsFeature implements IFeature {
this.command.dispose();
}

public setLanguageClient(languageClient: LanguageClient) {
this.languageClient = languageClient;
}

private launchAllTestsInActiveEditor(launchType: LaunchType, fileUri: vscode.Uri) {
const uriString = fileUri.toString();
const launchConfig = this.createLaunchConfig(uriString, launchType);
Expand Down
7 changes: 1 addition & 6 deletions src/features/RunCode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,9 @@ enum LaunchType {
Run,
}

export class RunCodeFeature implements IFeature {
export class RunCodeFeature implements vscode.Disposable {

private command: vscode.Disposable;
private languageClient: LanguageClient;

constructor(private sessionManager: SessionManager) {
this.command = vscode.commands.registerCommand(
Expand All @@ -31,10 +30,6 @@ export class RunCodeFeature implements IFeature {
this.command.dispose();
}

public setLanguageClient(languageClient: LanguageClient) {
this.languageClient = languageClient;
}

private async launchTask(
runInDebugger: boolean,
scriptToRun: string,
Expand Down
27 changes: 18 additions & 9 deletions src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ const AI_KEY: string = "AIF-d9b70cd4-b9f9-4d70-929b-a071c400b217";
let logger: Logger;
let sessionManager: SessionManager;
let extensionFeatures: IFeature[] = [];
let commandRegistrations: vscode.Disposable[] = [];
let telemetryReporter: TelemetryReporter;

const documentSelector: DocumentSelector = [
Expand Down Expand Up @@ -139,26 +140,30 @@ export function activate(context: vscode.ExtensionContext): void {
PackageJSON.version,
telemetryReporter);

// Create features
extensionFeatures = [
new ConsoleFeature(logger),
// Register commands that do not require Language client
commandRegistrations = [
Copy link
Contributor

@rkeithhill rkeithhill May 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not super crazy about having two different types of commands. What if rather than implementing IFeature, you create a base class called Feature (or another name if you can come up with something better). Then each subclass would use extends instead of implements. The base class would implement vscode.Disposable and would provide a protected languageClient property getter. Then we can treat each command/feature the same with respect to storing them in a single collection in main and a single loop to dispose of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree.
Having a separate list, means less work to do for the SessionManager, which only needs to manage classes, which require management therefore due to it depending on the languageClient. The value of this PR is to decouple standalone and self-contained command registrations, whilst also removing boilerplate code in those classes.
IFeature could be called something like ILanguageClientConsumer to better reflect what it does.

Copy link
Member

@TylerLeonhardt TylerLeonhardt May 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure how I feel about this but I do like the idea of having a base class for LanguageClientConsumer that sets a property languageClient.

export class LanguageClientConsumer {
    private _languageClient: LanguageClient;

    public get languageClient(): LanguageClient {
        if (!_languageClient) {
              // Show warning that extension isn't done starting up
        }
        return _languageClient;
    }

    public set languageClient(value: LanguageClient){
        this._languageClient = value;
    }
}

This will stop so many issues that just have the unhelpful Unable to instantiate; language client undefined.

Copy link
Member

@TylerLeonhardt TylerLeonhardt May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think I'm cool with some "features" just implementing Dispose and other "features" extending this LanguageClientConsumer.

Did you want to make that change @bergmeister?

the if statement can just run:

window.showInformationMessage("PowerShell extension has not finished starting up. Please try again in a few moments.");

or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just added a LanguageClientConsumer base class in 57b714b
I couldn't remove the usage of IFeature yet because the dispose() and setLanguageClient methods are still being used. Trying to get rid of that would be a bit more complicated and I wouldn't like to convolute this PR too much.

Copy link
Member

Choose a reason for hiding this comment

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

@bergmeister any reason: LanguageClientConsumer can't inherit from IFeature instead of everything inheriting from IFeature?

Copy link
Member

@TylerLeonhardt TylerLeonhardt Jun 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

LanguageClientConsumer could be abstract so each thing can implement their own dispose... but then you can remove all of the:

 public setLanguageClient(languageClient: LanguageClient) {
        this.languageClient = languageClient;
    }

and have just one of those in LanguageClientConsumer

Copy link
Contributor Author

@bergmeister bergmeister Jun 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, good idea. I made it an abstract class and dispose() an abstract method so that every implementation has to implement that one. I've also noticed that ExpandAlias and ShowHelpFeature have the logger passed into the constructor but actually never use the loggers, should we remove those unnecessary references as well or do that in another follow up PR?

Copy link
Member

Choose a reason for hiding this comment

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

Ah let's leave those in for now. Really we should add logging ;)

new ExamplesFeature(),
new OpenInISEFeature(),
new GenerateBugReportFeature(sessionManager),
new ISECompatibilityFeature(),
new OpenInISEFeature(),
new PesterTestsFeature(sessionManager),
new RunCodeFeature(sessionManager),
new CodeActionsFeature(logger),
new SpecifyScriptArgsFeature(context),
]

// Create features that require language client
extensionFeatures = [
new ConsoleFeature(logger),
new ExpandAliasFeature(logger),
new GetCommandsFeature(logger),
new ISECompatibilityFeature(),
new ShowHelpFeature(logger),
new FindModuleFeature(),
new PesterTestsFeature(sessionManager),
new RunCodeFeature(sessionManager),
new ExtensionCommandsFeature(logger),
new CodeActionsFeature(logger),
new NewFileOrProjectFeature(),
new RemoteFilesFeature(),
new DebugSessionFeature(context, sessionManager, logger),
new PickPSHostProcessFeature(),
new SpecifyScriptArgsFeature(context),
new HelpCompletionFeature(logger),
new CustomViewsFeature(),
new PickRunspaceFeature(),
Expand Down Expand Up @@ -206,6 +211,10 @@ export function deactivate(): void {
feature.dispose();
});

commandRegistrations.forEach((commandRegistration) => {
commandRegistration.dispose();
});

// Dispose of the current session
sessionManager.dispose();

Expand Down