GeezerVison

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping Sheetal Nandi (@sheetalkamat), Andrew Casey (@amcasey), Matt Bierner (@mjbvz), Mine Starks (@minestarks) for you. Feel free to loop in other consumers/maintainers if necessary

Matt Bierner (@mjbvz)
Please take a look about protocol or naming.
Thanks!

Basically work like this.

TypeScript Bot (@typescript-bot) pack this.

Heya Wenlu Wang (@Kingwl), I've started to run the tarball bundle task on this PR at d2fbd1e. You can monitor the build here.

Hey Wenlu Wang (@Kingwl), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/92086/artifacts?artifactName=tgz&fileId=5999C09B7BA4F0275D38BC9FCAC3474C22A8EDBA92BDD336E0A2658DE15F7C1B02&fileName=/typescript-4.2.0-insiders.20201227.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

The TypeScript team hasn't accepted the linked issue #42073. If you can get it accepted, this PR will have a better chance of being reviewed.

Should identifier be a literal argument?

Well, let me think about some examples:

f(1);
f(1 + 2);

f("Some String");
f("Some String".toLowerCase());
f(someString);
f(someString.toLowerCase());

f([0, 1, 2]);
f([0, 1, 2].map(processNumber));
f(integers);
f(integers.map(processNumber));

In these cases, I feel like the literal arguments and the expressions made up of literals convey the same amount of semantic information. I don’t think the + 2 or the .toLowerCase() or the .map(processNumber) does anything to negate the value of the parameter name hint.

That said, someString vs. someString.toLowerCase() and integers vs. integers.map(processNumber) also feel like they convey about the same amount of information. I’m fine just doing the same thing as C#, but the ParenthesizedExpression example in the test looked very strange to me. I think it would be appropriate to skipParentheses since we generally recognize that parens have no semantic meaning. But it’s not a big deal. Maybe someone would find it valuable to be able to force a hint to show up by parenthesizing an identifier. It’s not a blocker from me, just curious what the rationale was.

Thanks for the review. I've updated followed the comments.

I haven't tried this out yet to see how it actually plays. I assume I need vscode insiders?

Jesse Trinity (@jessetrinity) you need a local build of microsoft/vscode#113412

Thanks!

Thanks for creating such a great feature!

I have a question, why is there a limit on the hint length? Specifically, why is maxHintsLength set to 30? Are there any plans to change it sometime?