(#1015) Path exists check retains path scope specified by user, adds semi-colon to end of path #1017
Conversation
|
Looking for feedback on the following possible additional changes:
|
Also path exists check finds path whether or not trailing slash matches requested path, added alias 'path' for 'pathtoinstall' to align with powershell parameter naming, used chocolatey method Update-SessionEnvironment instead of simple path concetanation. Implements a couple aliases.
| #array drops blanks (one of which is always created by final semi-colon) | ||
| $actualPathArray = (Get-EnvironmentVariable -Name 'Path' -Scope $pathType -PreserveVariables).split(';',[System.StringSplitOptions]::RemoveEmptyEntries) | ||
| #checks for match with and without trailing slash | ||
| if (($actualpathArray -inotcontains $pathToInstall.ToLower()) -AND ($actualpathArray -inotcontains "$(($pathToInstall + '\').ToLower())")) |
There was a problem hiding this comment.
$actualpathArray is compared with lower case version of $pathToInstall. Where $actualpathArray is converted to lower case?
There was a problem hiding this comment.
The "i" in "-inotcontains" causes the entire comparison operation to be "case Insensitive". Technically this make "ToLower()" redundant.
There was a problem hiding this comment.
Oh, it makes sense. I am not very good at PS and was deceived by ToLower().
|
@DarwinJS I think from talking with @ferventcoder about this on Gitter Write-Host is best because it avoids major pitfalls of Write-Output's behavior, if you have Write-Output inside a function it automatically forces itself into the return value. There is a new Write-Information, but that is a PS5 only function iirc. |
|
Trying to understand why this isn't getting merged? |
|
Apologies @DarwinJS - this change wasn't under vNext, so it didn't get in for 0.10.4 - 0.10.5 was a bug fix release for a gating issue in 0.10.4. 0.10.6 is fixing an issue that came in from a change to how xml files are saved. It will make it in, and I did take a look and notice it may change the current behavior of looking at the entire path (breaking change), even if that is technically the right thing to do, and realized this changes a lot of code so it requires more scrutiny. Since it wasn't technically scheduled for the next release, it did not get merged. If we can agree that this is more of an annoyance than a critical fix - it may add an item to path twice, but it won't necessarily stop choco from working. HTH |
|
@ferventcoder - no need to apologize - I am just trying to understand. |
|
Has there been any progress on getting this merged? I just ran in to the issue described in #1015, where the Machine PATH was silently not getting updated because the path I was trying to add already existed on the User PATH. I never would have guessed what the problem was without finding that bug. |
|
|
gep13
changed the title
|
Thanks for the contribution here, and apologies that this was not reviewed for a long while. This has been resolved during the rewrite of this command to C# which shipped in 2.3.0 and the issue this seeks to resolve is no longer present; see here: #1015 (comment) As such, I'll be closing this PR, but thank you for the contribution nonetheless! 💜 |
(GH 1015) Path exists check retains path scope specified by user, adds semi-colon to end of path
Also path exists check finds path whether or not trailing slash matches requested path, added alias 'path' for 'pathtoinstall' to align with powershell parameter naming, used chocolatey method Update-SessionEnvironment instead of simple path concetanation.
#1015