FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Session: don't return undefined if a response is required · Pull Request #17165 · microsoft/TypeScript · GitHub

/ TypeScript Public

Session: don't return undefined if a response is required - #17165

Merged
6 commits merged into
masterfrom
requiredResponse
Aug 9, 2017
ConversationCommits (6)ChecksFiles changed
Merged

Session: don't return undefined if a response is required#17165
6 commits merged into
masterfrom
requiredResponse

Conversation

Deleted user (ghost) commented Jul 13, 2017

Copy link
Copy Markdown

Sequel to #16773
This doesn't yet fix SignatureHelp, since that doesn't return an array. I'm not sure if VSCode is depending on us sending back an "error" response if the user isn't currently at a signature.

ghost requested a review from Matt Bierner (mjbvz) July 13, 2017 15:33
Ron Buckton (rbuckton) suggested changes Aug 8, 2017
View reviewed changes
Comment thread src/server/session.ts Outdated
const definitions = project.getLanguageService().getDefinitionAtPosition(file, position);
if (!definitions) {
return undefined;
return [];

Ron Buckton (rbuckton) Aug 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Are these results mutable? Can we use a shared emptyArray construct?

Comment thread src/server/session.ts Outdated
}

private getFileAndProject(args: protocol.FileRequestArgs, errorOnMissingProject = true) {
private getFileAndProject(args: protocol.FileRequestArgs, errorOnMissingProject = true): { file: NormalizedPath, project: Project } {

Ron Buckton (rbuckton) Aug 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do we need the return type defined here, since it is correctly inferred?

Ron Buckton (rbuckton) commented Aug 8, 2017

Copy link
Copy Markdown
Contributor

Looks like the build is failing. Can you take a look?

Deleted user (ghost) commented Aug 8, 2017

Copy link
Copy Markdown
Author

Fix is #17685, will merge once that's in

Deleted user (ghost) commented Aug 8, 2017

Copy link
Copy Markdown
Author

Ron Buckton (@rbuckton) Build is passing now, good to go?

ghost merged commit f124e19 into master Aug 9, 2017
ghost deleted the requiredResponse branch August 9, 2017 20:46
Ben Lichtman (uniqueiniquity) pushed a commit to uniqueiniquity/TypeScript that referenced this pull request Aug 9, 2017
Session: don't return undefined if a response is required (microsoft#…
24d376b 
…17165)

* Session: don't return undefined if a response is required

* Use ReadonlyArray and emptyArray

* Remove inferred return type
Andrew Casey (amcasey) reviewed Aug 10, 2017
View reviewed changes
Comment thread src/server/session.ts
return {
info: renameInfo,
locs: []
locs: emptyArray

Andrew Casey (amcasey) Aug 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Missed one on line 812?

Andrew Casey (amcasey) reviewed Aug 10, 2017
View reviewed changes
Comment thread src/server/session.ts
if (simplifiedResult) {
const nameInfo = defaultProject.getLanguageService().getQuickInfoAtPosition(file, position);
if (!nameInfo) {
return undefined;

Andrew Casey (amcasey) Aug 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Might break deserialization.

Andrew Casey (amcasey) reviewed Aug 10, 2017
View reviewed changes
Comment thread src/server/session.ts

const completions = project.getLanguageService().getCompletionsAtPosition(file, position);
if (!completions) {
return undefined;

Andrew Casey (amcasey) Aug 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Might break deserialization.

Andrew Casey (amcasey) reviewed Aug 10, 2017
View reviewed changes
Comment thread src/server/session.ts
}

private getTypeDefinition(args: protocol.FileLocationRequestArgs): protocol.FileSpan[] {
private getTypeDefinition(args: protocol.FileLocationRequestArgs): ReadonlyArray<protocol.FileSpan> {

Andrew Casey (amcasey) Aug 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Line 627?

Andrew Casey (amcasey) reviewed Aug 10, 2017
View reviewed changes
Comment thread src/server/session.ts
}

private getDocumentHighlights(args: protocol.DocumentHighlightsRequestArgs, simplifiedResult: boolean): protocol.DocumentHighlightsItem[] | DocumentHighlights[] {
private getDocumentHighlights(args: protocol.DocumentHighlightsRequestArgs, simplifiedResult: boolean): ReadonlyArray<protocol.DocumentHighlightsItem> | ReadonlyArray<DocumentHighlights> {

Andrew Casey (amcasey) Aug 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Line 717?

ghost mentioned this pull request Aug 10, 2017
Merged

Andrew Casey (amcasey) commented Aug 10, 2017

Copy link
Copy Markdown
Member

Andy (Andrewkraft) (@Andy-MS) It might be safest to revert this (and hold off on #17727) until we further discuss the implications for clients. (cc: Mine Starks (@minestarks), Ron Buckton (@rbuckton))

Deleted user (ghost) commented Aug 10, 2017

Copy link
Copy Markdown
Author

Might be better to wait for release-2.5 and remove this from that, but not from master.

Andrew Casey (amcasey) commented Aug 11, 2017

Copy link
Copy Markdown
Member

I think release-2.5 exists now. The plan is to revert this there and submit #17728 to master (and close #17727)?

ghost mentioned this pull request Aug 14, 2017
Merged
Microsoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Matt Bierner (mjbvz) Awaiting requested review from mjbvz

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL