Expand env-vars in background image path #3203
Conversation
Co-Authored-By: Dustin L. Howett (MSFT) <duhowett@microsoft.com>
* Set Terminal Settings' Background Iamge Path to expanded Profile's Background Image Path * Removed Profile's superfluous Background Image Path setter * Tidied up some loose ends and comments
| @@ -1,33 +1,40 @@ | |||
| # Welcome | |||
|
|
|||
| This repository contains the source code for: | |||
There was a problem hiding this comment.
ABSOLUTELY NOT my man, you've gotta make sure your branches are clean before starting ;)
There was a problem hiding this comment.
Damnit! Just saw. Apologies. Will kill and re-create. Seems my home dev box is unclean.
DHowett-MSFT
left a comment
DHowett-MSFT
left a comment
There was a problem hiding this comment.
Other than the tests not testing anything, I'm doing an "approve with suggestions"
|
|
||
| VerifyParseSucceeded(settingsJson); | ||
| CascadiaSettings settings; | ||
| settings._ParseJsonString(settingsJson, false); |
There was a problem hiding this comment.
i like that you added tests, but you should validate that they actually did something! check the resulting values in the path variables.
since you can't really make sure WINDIR expanded to "the right value", you could "check whether there is a % in the final string" maybe?
|
You should force push this PR instead of deleting it and making a new one! You can even just revert your readme changes in this branch. |
Summary of the Pull Request
Expands any environment variables in background image file paths so that users can specify location of background images relative to, for example, %USERPROFILE%
References
PR Checklist
Validation Steps Performed
Thoroughly debugged with previous failing case, now working case, empty strings, etc. Also added basic regression tests that will fail if icon or backgroundImage paths contain env-vars and the path expansion mechanisms are ever removed/subverted.