-
Notifications
You must be signed in to change notification settings - Fork 646
move env consolidation after loading envFile #935
Conversation
@goenning, |
if (value.length > 0 && value.charAt(0) === '"' && value.charAt(value.length - 1) === '"') { | ||
value = value.replace(/\\n/gm, '\n'); | ||
} | ||
env[r[1]] = value.replace(/(^['"]|['"]$)/g, ''); |
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.
Actually, just changing the env
in this line to finalEnv
should also do the trick. Can you try that?
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.
Or we can get rid of finalEnv
altogether and instead just replace that with
env = Object.assign({}, process.env, env)
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.
Great idea! I was focusing only on getting it done 😄 Can you see how it looks now?
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.
By the way, I'm suggesting that we use the following precedence order
launch env -> file env -> process env
so we can override file env values with launch specific values.
What do you think? Check the last commit.
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.
agreed on the order.
src/debugAdapter/goDebug.ts
Outdated
@@ -181,7 +181,7 @@ class Delve { | |||
this.program = program; | |||
this.remotePath = remotePath; | |||
let mode = launchArgs.mode; | |||
let env = launchArgs.env || {}; | |||
let env = Object.assign({}, launchArgs.env, process.env); |
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.
this way the process.env
overrides launchArgs.env
It should be the other way around
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.
Check last commit, I changed it.
if (value.length > 0 && value.charAt(0) === '"' && value.charAt(value.length - 1) === '"') { | ||
value = value.replace(/\\n/gm, '\n'); | ||
} | ||
env[r[1]] = value.replace(/(^['"]|['"]$)/g, ''); |
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.
agreed on the order.
great, thanks @goenning! |
Works fine for me after I moved this :)
Fixes #934