Skip to content

MultistepOperation: Don't need 'completed', just use requestId === undefined#17173

Merged
3 commits merged into
masterfrom
completed
Aug 8, 2017
Merged

MultistepOperation: Don't need 'completed', just use requestId === undefined#17173
3 commits merged into
masterfrom
completed

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jul 13, 2017

If !this.completed, this.requestId should always be set. And there's no need to leave requestId set if the request is completed. So we can combine these into one variable.

@ghost ghost requested a review from sheetalkamat July 13, 2017 17:42
Comment thread src/server/session.ts Outdated
this.operationHost.sendRequestCompletedEvent(this.requestId);
}
this.completed = true;
if (this.requestId) {
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.

Is it possible for a request to have an id of 0? It might be more reliable to test this.requestId !== undefined

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It looks like the server is responsible for sending us these IDs, so it might have happened to always be non-zero when coming from VSCode, but best not to rely on that.

@rbuckton
Copy link
Copy Markdown
Contributor

rbuckton commented Aug 8, 2017

Can you resolve the merge conflict and the comment above?

Copy link
Copy Markdown
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.

Leave a comment

@ghost ghost merged commit e1802f4 into master Aug 8, 2017
@ghost ghost deleted the completed branch August 8, 2017 17:49
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants