Skip to content

Conversation

@orta
Copy link
Contributor

@orta orta commented May 7, 2021

Fixes #43350 by allowing checking for the existence of a source file instead of limiting via filename.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 7, 2021
const request = this.processRequest<protocol.EncodedSemanticClassificationsRequest>(protocol.CommandTypes.EncodedSemanticClassificationsFull, { file, start: span.start, length: span.length, format });
const r = this.processResponse<protocol.EncodedSemanticClassificationsResponse>(request);
return r.body!;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the actual call which vscode uses, so I've replicated that codepath for the server tests. The non-encoded versions of the APIs aren't in the server, and it doesn't feel right to add them just for a test.

if (actual.spans.length !== expected) {
this.raiseError(`encodedSemanticClassificationsLength failed - expected total spans to be ${expected} got ${actual.spans.length}`);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I can use the 'encoded' classifications routes in the server

if (!isTsOrTsxFile(fileName)) {
synchronizeHostData();
const sourceFile = program && program.getSourceFile(fileName);
if (!sourceFile) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moves synchronizeHostData to the start of the function, then checks for the program and sourceFile. Only potential behavior change is that getValidSourceFile would previously have thrown when there is no source file and now it would return empty results.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should still keep getValidSourceFile as calling this on wrong LS is still error...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, done!


// This test validates that an arbitrary JS file gets
// encoded semantic classifications when requested
verify.encodedSemanticClassificationsLength("2020", 9)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above test is moved to the server subfolder, where it now goes through the same codepaths as vscode would

@orta orta requested review from andrewbranch and sheetalkamat May 7, 2021 15:02
@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 7, 2021
Copy link
Member

@sheetalkamat sheetalkamat left a comment

Choose a reason for hiding this comment

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

I think we should still keep getValidSourceFile call.. Other change of removing if statement that checks not TS is correct

@orta orta merged commit 6bb481c into microsoft:master May 7, 2021
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: Team For Milestone Bug PRs that fix a bug with a specific milestone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

encodedSemanticClassifications-full returns empty results for JavaScript file

3 participants