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

[bergmeister]

PR Summary

IFeature is a really bad name for declaring usage of the language client, we should maybe rename that in another PR.
Also cleanup some unused imports in those classes.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

• PR has a meaningful title
• Summarized changes
• PR has tests
• This PR is ready to merge and is not work in progress
  • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

[rkeithhill]

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.

[bergmeister]

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.

[TylerLeonhardt]

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.

[TylerLeonhardt]

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.

[bergmeister]

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.

[TylerLeonhardt]

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

[TylerLeonhardt]

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

[bergmeister]

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?

[TylerLeonhardt]

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

[TylerLeonhardt]

@bergmeister was this ready for review again? Unfortunately I added #2799 and #2789 which added 2 more IFeatures so if you don't mind changing that, and dealing with the tiny conflict

[bergmeister]

Yes, ready for review, adapted for upstream changes.