-
Notifications
You must be signed in to change notification settings - Fork 447
Conversation
@@ -110,14 +110,16 @@ export class TfvcExtension { | |||
const right: Uri = TfvcSCMProvider.GetRightResource(resource); | |||
const title: string = resource.GetTitle(); | |||
|
|||
if (!right) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this change actually helps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logically, it doesn't. It just keeps us aligned with the similar changes in the Git extension.
src/tfvc/tfvc-extension.ts
Outdated
} | ||
|
||
return commands.executeCommand<void>("vscode.diff", left, right, title); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably want to add an await here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, certainly to be consistent. I'm a little under-informed about when to 'return await' on a function that returns a Promise while you're inside of a function that's async. I've read in several places that you shouldn't actually need it as it can/does create an additional promise (the await I'll be adding here). The act of returning should also create a promise. But, again, I'll need to read up more to get clarity. And, of course, there's a linting rule for this. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh Yeah!
This prevents the undefined error we were seeing previously but only allows either Open Diff or Open File (not both). Will need to debug later.