-
Notifications
You must be signed in to change notification settings - Fork 451
Conversation
package.json
Outdated
"tfvc.location": { | ||
"type": "string", | ||
"default": "", | ||
"description": "Specify the full path to the TF executable to use for TFVC functionality." |
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.
should we change "TF executable" to "TF executable or script"? Doesn't matter much, but since we do support tf.cmd, it might be more "correct" to include script.
src/team-extension.ts
Outdated
this._gitClient.OpenFileHistory(this._manager.RepoContext); | ||
} else if (this._manager.RepoContext.Type === RepositoryType.TFVC) { | ||
let editor = window.activeTextEditor; |
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.
consider whether this should simply be "this._manager.Tfvc.TfvcViewHistory()" and all the logic (under the TFVC else branch) be inside there. This would just keep this file a bit smaller.
src/tfvc/tfvc-extension.ts
Outdated
if (!itemPath) { | ||
this._manager.Telemetry.SendEvent(TfvcTelemetryEvents.OpenRepositoryHistory); | ||
//Just display the history url of the entire repo | ||
Utils.OpenUrl(this._manager.RepoContext.RemoteUrl + "_versionControl?_a=history"); |
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.
Does this assume that the RemoteUrl ends in "/"? Do we have a helper that join url paths like path.combine? That would be safer.
src/tfvc/tfvc-extension.ts
Outdated
this._manager.Telemetry.SendEvent(TfvcTelemetryEvents.OpenFileHistory); | ||
let serverPath: string = itemInfos[0].serverItem; | ||
let file: string = encodeURIComponent(serverPath); | ||
Utils.OpenUrl(this._manager.RepoContext.RemoteUrl + "_versionControl?path=" + file + "&_a=history"); |
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.
same as above about remoteUrl.
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.
some minor comments, but looks good!
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.
LGTM
No description provided.