Adding CLC version checks as well as localizing #72
Conversation
jpricket
commented
Jan 30, 2017
jpricket
commented
- removing some TODOs from the code
- Checking for the min version of the CLC
- Checking to make sure the TF.cmd file exists
- Localizing strings
- Added version class and unit tests
|
Hi @jpricketMSFT, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
| @@ -18,19 +18,23 @@ var _ = require("underscore"); | |||
| * by the repositoryRootFolder. | |||
| */ | |||
| export class Repository { | |||
There was a problem hiding this comment.
some of these changes are just renaming the private variables to have "_"
| // TFVC messages/errors | ||
| static ChooseItemQuickPickPlaceHolder: string = "Choose a file to open the file."; | ||
| static TfvcLocationMissingError: string = "The path to the TFVC command line was not found in the user settings. Please set this value (tfvc.location) and try again."; | ||
| static TfMissingError: string = "Unable to find the TF executable at the expected location. Please set verify the installation and location of TF. Expected path: "; |
There was a problem hiding this comment.
"set verify"... "set"? "verify"? "set or verify"?
There was a problem hiding this comment.
I should read these things first :(
| new GetVersion()); | ||
| public async CheckVersion(): Promise<void> { | ||
| if (!this._versionAlreadyChecked) { | ||
| this._versionAlreadyChecked = true; |
There was a problem hiding this comment.
I'd do the version check before setting _versionAlreadyChecked to true.
There was a problem hiding this comment.
So, CheckVersion will throw the error if the version isn't correct. And I don't want to throw it twice, so I need to set the boolean before it might throw. I will add a comment.
| // check to make sure that the file exists in that location | ||
| const stats: any = fs.lstatSync(this._tfvcPath); | ||
| if (!stats || !stats.isFile()) { | ||
| throw new TfvcError({ |
There was a problem hiding this comment.
We should chat about our understanding of how TfvcError works. It is a way to create objects that we can throw but we can't catch(){} a particular type... which reduces its effectiveness. Nothing to do for this comment, just FYI.
| @@ -48,5 +48,12 @@ export class Strings { | |||
| static StatusCode401: string = "Unauthorized. Check your authentication credentials and try again."; | |||
| static StatusCodeOffline: string = "It appears Visual Studio Code is offline. Please connect and try again."; | |||
| static ProxyUnreachable: string = "It appears the configured proxy is not reachable. Please check your connection and try again."; | |||
|
|
|||
| // TFVC messages/errors | |||
| static ChooseItemQuickPickPlaceHolder: string = "Choose a file to open the file."; | |||
There was a problem hiding this comment.
"Choose a file to open..."... "it"? "file to open the file" doesn't ready very well (to me).
There was a problem hiding this comment.
Strange. I don't remember typing it that way, but I will change it.
| try { | ||
| await repo.CheckVersion(); | ||
| } catch (err) { | ||
| VsCodeUtils.ShowWarningMessage(err); |
There was a problem hiding this comment.
I think you want err.message here.
