Closed WHYBBE closed 1 year ago
I think we can avoid making another function by just doing:
function Get-Env {
param(
[String] $name,
[Switch] $global
)
$RegisterKey = if ($global) {
Get-Item -Path 'HKLM:\SYSTEM\CurrentControlSet\Control\Session Manager'
} else {
Get-Item -Path 'HKCU:'
}
$EnvRegisterKey = $RegisterKey.OpenSubKey('Environment')
$RegistryValueOption = [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames
$EnvRegisterKey.GetValue($name, $null, $RegistryValueOption)
}
function Set-Env {
param(
[String] $name,
[String] $val,
[Switch] $global
)
$RegisterKey = if ($global) {
Get-Item -Path 'HKLM:\SYSTEM\CurrentControlSet\Control\Session Manager'
} else {
Get-Item -Path 'HKCU:'
}
$EnvRegisterKey = $RegisterKey.OpenSubKey('Environment', $true)
$RegistryValueKind = if ($EnvRegisterKey.GetValue($name)) {
$EnvRegisterKey.GetValueKind($name)
} else {
[Microsoft.Win32.RegistryValueKind]::ExpandString
}
$EnvRegisterKey.SetValue($name, $val, $RegistryValueKind)
}
I think we can avoid making another function by just doing:
function Get-Env { param( [String] $name, [Switch] $global ) $RegisterKey = if ($global) { Get-Item -Path 'HKLM:\SYSTEM\CurrentControlSet\Control\Session Manager' } else { Get-Item -Path 'HKCU:' } $EnvRegisterKey = $RegisterKey.OpenSubKey('Environment') $RegistryValueOption = [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames $EnvRegisterKey.GetValue($name, $null, $RegistryValueOption) } function Set-Env { param( [String] $name, [String] $val, [Switch] $global ) $RegisterKey = if ($global) { Get-Item -Path 'HKLM:\SYSTEM\CurrentControlSet\Control\Session Manager' } else { Get-Item -Path 'HKCU:' } $EnvRegisterKey = $RegisterKey.OpenSubKey('Environment', $true) $RegistryValueKind = if ($EnvRegisterKey.GetValue($name)) { $EnvRegisterKey.GetValueKind($name) } else { [Microsoft.Win32.RegistryValueKind]::ExpandString } $EnvRegisterKey.SetValue($name, $val, $RegistryValueKind) }
Originally, I wanted to do the same, but the definition of RegisterKey was repeated. I think it was not good. At the same time, I don't want to add another parameter to the original Get-Env
, which is the same as the env in Scoop, so I modified it like this. What do you think?
Originally, I wanted to do the same, but the definition of RegisterKey was repeated. I think it was not good.
RegisterKey
variable here has function scope, so they won't interfere with each other.
At the same time, I don't want to add another parameter to the original
Get-Env
, which is the same as the env in Scoop, so I modified it like this. What do you think?
Get-Env
in the installer still has only 2 parameters, right? (same as before)
Originally, I wanted to do the same, but the definition of RegisterKey was repeated. I think it was not good.
RegisterKey
variable here has function scope, so they won't interfere with each other.At the same time, I don't want to add another parameter to the original
Get-Env
, which is the same as the env in Scoop, so I modified it like this. What do you think?
Get-Env
in the installer still has only 2 parameters, right? (same as before)
Yes, there will be no interference between them, but I am just afraid that there will be a next revision, so the next revision will have to change two places, which doesn't look very good.
No, what I mean by that sentence is to make GetEnv
the same as the env
function in Scoop, and use the third parameter to distinguish whether it is get or set, but it still needs to change the name, which is not very good.
All in all, hurry up and merge these two PRs. I don’t think there will be a better solution if we continue to discuss. Just choose a method that everyone agrees with. (I think the code you provided above is ok, as long as others think it's ok too.)
Adding Get-Env
and Set-Env
as separate functions should be preferred over copying over the old hacky way with $val -eq '__get'
.
I'll review this in these two days. Thanks for the contribution.
The implementation looks good to me. CI code linting failed because of the name of the newly introduced function. To fix it either change to use a verb that does not require UseShouldProcessForStateChangingFunctions
or implement ShouldProcess
for Set-Env
.
Put
is not an approved Cmdlet verb, you may use one from the Get-Verb
list. Use
might be a proper one in this case.
Put
is not an approved Cmdlet verb, you may use one from theGet-Verb
list.Use
might be a proper one in this case.
I chose Write
, and if other people think it is not suitable, you can also give some suggestions from the few verbs I listed.
e.g., Enter
, Join
, Write
, Edit
, Publish
, Save
, Install
, Register
, Submit
...
Use a method of obtaining environment variables that does not expand
%%
, while maintaining the original type of environment variables, such asREG_EXPAND_SZ
,REG_SZ
.Same issue related to ScoopInstaller/Scoop#5394 Some important content in ScoopInstaller/Scoop#5395