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.

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

🧹Cleanup unused code #2686

Merged
merged 1 commit into from
May 11, 2020
Merged

🧹Cleanup unused code #2686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 0 additions & 2 deletions src/features/GenerateBugReport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
* Copyright (C) Microsoft Corporation. All rights reserved.
*--------------------------------------------------------*/

import cp = require("child_process");
import os = require("os");
import vscode = require("vscode");
import { IFeature, LanguageClient } from "../feature";
Expand Down Expand Up @@ -30,7 +29,6 @@ const extensions =
export class GenerateBugReportFeature implements IFeature {

private command: vscode.Disposable;
private powerShellProcess: cp.ChildProcess;

constructor(private sessionManager: SessionManager) {
this.command = vscode.commands.registerCommand("PowerShell.GenerateBugReport", () => {
Expand Down
2 changes: 0 additions & 2 deletions src/features/HelpCompletion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ class TriggerFinder {

class HelpCompletionProvider {
private triggerFinderHelpComment: TriggerFinder;
private lastChangeText: string;
private lastChangeRange: Range;
private lastDocument: TextDocument;
private langClient: LanguageClient;
Expand All @@ -148,7 +147,6 @@ class HelpCompletionProvider {

public updateState(document: TextDocument, changeText: string, changeRange: Range): void {
this.lastDocument = document;
this.lastChangeText = changeText;
this.lastChangeRange = changeRange;
this.triggerFinderHelpComment.updateState(document, changeText);
}
Expand Down
5 changes: 0 additions & 5 deletions src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,6 @@ import utils = require("./utils");
// tslint:disable-next-line: no-var-requires
const PackageJSON: any = require("../../package.json");

// NOTE: We will need to find a better way to deal with the required
// PS Editor Services version...
const requiredEditorServicesVersion = "2.0.0";
Copy link
Member

Choose a reason for hiding this comment

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

This isn't used anywhere, right?

Copy link
Contributor Author

@bergmeister bergmeister May 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

if it was used by Typescript, then the build would've been red. It was indirectly used by the PowerShell release function that I removed in this PR as well. I believe this is an old artefact from the days when the preview was using PSES v2 but the RTM version was still on PSES v1. I cc'd @rjmholt to confirm this though

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right @bergmeister


// the application insights key (also known as instrumentation key) used for telemetry.
const AI_KEY: string = "AIF-d9b70cd4-b9f9-4d70-929b-a071c400b217";

Expand Down Expand Up @@ -132,7 +128,6 @@ export function activate(context: vscode.ExtensionContext): void {

sessionManager =
new SessionManager(
requiredEditorServicesVersion,
logger,
documentSelector,
PackageJSON.displayName,
Expand Down
9 changes: 3 additions & 6 deletions src/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ export class SessionManager implements Middleware {
private sessionSettings: Settings.ISettings = undefined;
private sessionDetails: utils.IEditorServicesSessionDetails;
private bundledModulesPath: string;
private telemetryReporter: TelemetryReporter;

// Initialized by the start() method, since this requires settings
private powershellExeFinder: PowerShellExeFinder;
Expand All @@ -66,18 +65,16 @@ export class SessionManager implements Middleware {
vscode.env.sessionId === "someValue.sessionId";

constructor(
private requiredEditorServicesVersion: string,
private log: Logger,
private documentSelector: DocumentSelector,
private hostName: string,
private version: string,
private reporter: TelemetryReporter) {
hostName: string,
version: string,
private telemetryReporter: TelemetryReporter) {
Copy link
Member

Choose a reason for hiding this comment

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

Did you want to do this for hostname and version above it too?

I actually started the opposite change in debugAdapter.ts (by using underscore like we do in C#....) but I'm on the fence about it.

I feel conflicted about the inline private declaration in the constructors... It's less boilerplate, but not as discoverable IMO.

I'm fine making it consistent with what is used in the file now, but (eventually...) I may refactor one way or the other.

Copy link
Contributor Author

@bergmeister bergmeister May 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The reason why I removed the private inline field on hostName and version is because it is not used but rather directly assigned to the backing fields HostName and HostVersion and those properties get later used/modified. We should probably rename the version parameter to hostVersion if you agree? In terms of casing, I quite like the Upper casing and wouldn't use underscores TBH. The official TypeScript guide explicitly recommends against that: https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines

"Do not use "_" as a prefix for private properties."

JavaScript doesn't have a notion of private/public, therefore the private modifier in TypeScript is just some syntactic sugar to give a compilation error if the stated intent is not followed, hence why people probably only have either a public or private field and therefore don't need a prefix it seems.

The reason why I didn't remove the private inline field for telemetryReporter is because that private property is actually being used at the moment. Due to the this. accessor, the reader knows that it's a property and not local variable/parameter. To me, using the private auto-created property signifies that we use the passed in parameter directly as-is and not a potentially modified, cached version (yes, one could've technically modify that property but the lower case signals to me the intent of just using the passed in parameter as-is, maybe that's just me?).
Happy to go for whatever you prefer, if I read correctly, you'd prefer to create an explicit backing field telemetryReporter, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to eventually get consistent on whether we use ctror inline private (or public/protected) declarations. I lean towards using them since they do reduce boilerplate code and less code means less opportunity for bugs to creep in.


this.platformDetails = getPlatformDetails();

this.HostName = hostName;
this.HostVersion = version;
this.telemetryReporter = reporter;

const osBitness = this.platformDetails.isOS64Bit ? "64-bit" : "32-bit";
const procBitness = this.platformDetails.isProcess64Bit ? "64-bit" : "32-bit";
Expand Down
42 changes: 0 additions & 42 deletions tools/postReleaseScripts/updateExtensionVersions.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,23 +116,6 @@ function FindPackageJsonVersionSpan
throw 'Did not find package.json version field'
}

function FindRequiredPsesVersionSpan
{
param(
[Parameter(Mandatory)]
[string]
$MainTsContent
)

$pattern = [regex]'const\s+requiredEditorServicesVersion\s+=\s+"(.*)"'
$versionGroup = $pattern.Match($MainTsContent).Groups[1]

return @{
Start = $versionGroup.Index
End = $versionGroup.Index + $versionGroup.Length
}
}

function FindVstsBuildVersionSpan
{
param(
Expand All @@ -150,27 +133,6 @@ function FindVstsBuildVersionSpan
}
}

function UpdateMainTsPsesVersion
{
param(
[Parameter(Mandatory)]
[string]
$MainTsPath,

[Parameter(Mandatory)]
[version]
$Version
)

$mainTsContent = Get-Content -Raw $MainTsPath
$mainTsVersionSpan = FindRequiredPsesVersionSpan $mainTsContent
$newMainTsContent = New-StringWithSegment -String $mainTsContent -NewSegment $Version -StartIndex $mainTsVersionSpan.Start -EndIndex $mainTsVersionSpan.End
if ($newMainTsContent -ne $mainTsContent)
{
Set-Content -Path $MainTsPath -Value $newMainTsContent -Encoding utf8NoBOM -NoNewline
}
}

function UpdateDockerFileVersion
{
param(
Expand Down Expand Up @@ -247,16 +209,12 @@ if (-not $PRDescription)
}

# Get the marketplace/non-semver versions for various files
$psesVersion = Get-VersionFromSemVer -SemVer $NewVersion
$marketPlaceVersion = GetMarketplaceVersionFromSemVer -SemVer $NewVersion

# Finally create the new package.json file
$newPkgJsonContent = New-StringWithSegment -String $packageJson -NewSegment $NewVersion -StartIndex $pkgJsonVersionOffsetSpan.Start -EndIndex $pkgJsonVersionOffsetSpan.End
Set-Content -Path $paths.packageJson -Value $newPkgJsonContent -Encoding utf8NoBOM -NoNewline

# Create the new content for the main.ts required version file
UpdateMainTsPsesVersion -MainTsPath $paths.mainTs -Version $psesVersion

# Create the new content for the VSTS dockerfile
UpdateDockerFileVersion -DockerFilePath $paths.vstsDockerFile -Version $marketPlaceVersion

Expand Down