🧹Cleanup unused code #2686
🧹Cleanup unused code #2686
Conversation
| @@ -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"; | |||
There was a problem hiding this comment.
This isn't used anywhere, right?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think you're right @bergmeister
| private reporter: TelemetryReporter) { | ||
| hostName: string, | ||
| version: string, | ||
| private telemetryReporter: TelemetryReporter) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
PR Summary
As far as I see the PSES version hack for releasing it not needed any more. WDYT @rjmholt ?
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets.Please mark anything not applicable to this PR
NA.WIP:to the beginning of the title and remove the prefix when the PR is ready