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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose EncodedSemanticClassificationsRequest in protocol.d.ts #42640

Merged

Conversation

@orta
Copy link
Member

@orta orta commented Feb 4, 2021

Fixes #42587 - I think because this request used to have the same args as many other requests then it was not included in the protocol.d.ts.

Now it differs, so I've removed the internal markers 馃憤馃徎

@orta orta self-assigned this Feb 4, 2021
@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Feb 4, 2021

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 @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

/**
* A request to get encoded semantic classifications for a span in the file
*/
interface EncodedSemanticClassificationsRequest extends FileRequest {

This comment has been minimized.

This comment has been minimized.

@sheetalkamat

sheetalkamat Feb 4, 2021
Member

Aren鈥檛 they in typescript/tsserver .d.ts ?

This comment has been minimized.

@mjbvz

mjbvz Feb 4, 2021
Contributor

We currently only use protocol.d.ts with VS Code. I think this should have all the types used communicating from or to tsserver

This comment has been minimized.

@orta

orta Feb 5, 2021
Author Member

Makes sense, added

@@ -868,6 +866,14 @@ namespace ts.server.protocol {
format?: "original" | "2020"
}

/** The response for a EncodedSemanticClassificationsRequest */
export interface EncodedSemanticClassificationsResponse extends Response {
body?: {

This comment has been minimized.

@sheetalkamat

sheetalkamat Feb 5, 2021
Member

Pull this out as EncodedSemanticClassificationsResponseBody interface

export interface EncodedSemanticClassificationsResponse extends Response {
body?: {
endOfLineState: EndOfLineState;
spans: number[];

This comment has been minimized.

@sheetalkamat

sheetalkamat Feb 5, 2021
Member

Dont we need specific enum for classifications returned otherwise vscode would need separate list?

This comment has been minimized.

@orta

orta Feb 24, 2021
Author Member

Yeah, maybe, the spans looks like

     * @returns a number array encoded as triples of [start, length, ClassificationType, ...].

When it's the older format, so I've included it but it's not referenced up here

@sandersn sandersn added this to Not started in PR Backlog Feb 16, 2021
@mjbvz
Copy link
Contributor

@mjbvz mjbvz commented Feb 23, 2021

Hi, just want to check if this is on track for TS 4.2.1. If not, could we please get this into a recovery build of TS 4.2 instead of waiting for TS 4.3? This would let us better validate the semantic highlighting support API while switching to the native TS implementation (aeschli/typescript-vscode-sh-plugin#22)

@orta
Copy link
Member Author

@orta orta commented Feb 24, 2021

Just updating the baselines, I'll also submit this to the 4.2 branch 馃憤馃徎

@orta orta requested a review from sheetalkamat Feb 24, 2021
*/
export interface EncodedSemanticClassificationsResponseBody {
endOfLineState: EndOfLineState;
spans: ClassifiedSpan[] | number[];

This comment has been minimized.

@sheetalkamat

sheetalkamat Feb 25, 2021
Member

This isnt correct. The response currently from getEncodedSemanticClassifications returns type as ts.Classifications where spans is number[].
More over you are referencing ClassifiedSpan here which is only in services and not in protocol.
This doesnt correctly reflect what we return.

This comment has been minimized.

@orta

orta Feb 25, 2021
Author Member

Dang, I thought I reverted that, that's what I get for shipping late at night, Running baselines to update now and will double check before I ask for a re-review! Thanks

This comment has been minimized.

@orta

orta Feb 25, 2021
Author Member

OK, that looks good until we add some sort of wild syntax like spans: [...[number, number, ClassificationType][] ] heh

PR Backlog automation moved this from Not started to Waiting on author Feb 25, 2021
@orta orta requested a review from sheetalkamat Feb 25, 2021
PR Backlog automation moved this from Waiting on author to Needs merge Feb 25, 2021
@orta
Copy link
Member Author

@orta orta commented Feb 25, 2021

Cool, I'm going to replicate this for the 4.2 branch now

@orta orta merged commit a2f09ed into microsoft:master Feb 25, 2021
9 checks passed
9 checks passed
@github-actions
build (10.x)
Details
@github-actions
CodeQL-Build CodeQL-Build
Details
@github-actions
build (12.x)
Details
@github-actions
build (14.x)
Details
@github-code-scanning
CodeQL 1 analysis not found
Details
@microsoft-cla
license/cla All CLA requirements met.
Details
@azure-pipelines
node10 Build #96949 succeeded
Details
@azure-pipelines
node12 Build #96947 succeeded
Details
@azure-pipelines
node14 Build #96948 succeeded
Details
PR Backlog automation moved this from Needs merge to Done Feb 25, 2021
orta added a commit to orta/TypeScript that referenced this pull request Feb 25, 2021
鈥ft#42640)

* Expose EncodedSemanticClassificationsRequest in protocol.d.ts

* Adds the response for encoded semantic highlights too

* Update types:

* Also include classificationtype anyway

* Fix feedback
orta added a commit that referenced this pull request Feb 25, 2021
#42965)

* Expose EncodedSemanticClassificationsRequest in protocol.d.ts

* Adds the response for encoded semantic highlights too

* Update types:

* Also include classificationtype anyway

* Fix feedback
This was referenced Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
PR Backlog
  
Done
Linked issues

Successfully merging this pull request may close these issues.

5 participants