-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Changed ENV variable name to allow for custom paths #5813
feat: Changed ENV variable name to allow for custom paths #5813
Conversation
df78a51
to
117cfad
Compare
Rebased onto |
# Conflicts: # lib/core.ps1 # Conflicts: # lib/install.ps1
# Conflicts: # lib/core.ps1
1765925
to
d0f8ee7
Compare
Resolved the Merge conflicts |
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.
I'll submit a reviewed commit soon.
@@ -748,7 +756,7 @@ function env($name, $global, $val = '__get') { | |||
|
|||
if ($val -eq '__get') { | |||
$RegistryValueOption = [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames | |||
$EnvRegisterKey.GetValue($name, $null, $RegistryValueOption) | |||
$EnvRegisterKey.GetValue($name, $null, $RegistryValueOption) + "" |
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.
What's this ""
for?
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.
If the environment variable is empty we get a null exception later on. so we add the empty string to ensure it always returns a string even if the environment variable is empty.
@@ -1415,6 +1423,8 @@ if ($pathExpected) { | |||
} | |||
$scoopConfig = load_cfg $configFile | |||
|
|||
$scoop_path_env = get_config PATH_ENV 'PATH' |
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.
Recommend using a config option to check if use isolated path, and the PATH_ENV here could be SCOOP_PATH
.
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.
I assumed we would want the default behavior to revert to the current behavior of just adding the paths to PATH
. So we should have two different config options? A bool to enable custom paths and one to contain the path itself? I feel it makes more sense to just have a single config value that contains the Environment variable scoop should write to. That way we don't need extra checks and the user is always aware which environment variable is being written to.
Closing this in favour of #5840 |
Description
This PR adds the ability to define the
SCOOP_ENV
(Subject to change) environment variable which changes which environment variable scoop writes paths to. In my case I have all the scoop paths underSCOOP_PATH
which can get as clutters as it would like. while myPATH
just contains the reference to the scoop paths by containing%SCOOP_PATH%
.Motivation and Context
Fixes #1813
I was unable to edit my PATH environment variable through the windows dialogue as scoop had added more than a dozen different paths. The recommended answer was to create a different environment variable to contain all the paths & just link to it via %ENV_NAME%. I realized that Scoop did not have the ability to change the environment variable that the paths got added to so I just added it myself
How Has This Been Tested?
It has been running as my local scoop version for the last few weeks & has worked. I will probably add a Unit test if this is something the Scoop team considered
Checklist:
develop
branch.