-
Notifications
You must be signed in to change notification settings - Fork 77
Add setting for environment variables to send to Swift #305
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
Conversation
Otherwise we can send `{}` to `execFile` which messes with stuff.
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.
LGTM otherwise
@@ -77,6 +77,12 @@ const configuration = { | |||
get buildArguments(): string[] { | |||
return vscode.workspace.getConfiguration("swift").get<string[]>("buildArguments", []); | |||
}, | |||
/** Environment variables to set when building */ | |||
get swiftEnvironmentVariables(): { [key: string]: string } { |
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 guess we can extract Object.keys(swiftEnvironmentVariables).length > 0
in to a new computed property?
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.
Not sure this is totally necessary, I do the same in execFile to make it more obvious why we are setting runtimePath env.
when adding environment vars for swiftExec or execFile we either combine with vars passed in or the current process vars
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.
One minor spelling, otherwise LGTM
src/utilities/utilities.ts
Outdated
options.env = { ...options.env, ...runtimeEnv }; | ||
} | ||
} | ||
// it options contain environment keys add them to process environment keys |
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.
Not spelling
// it options contain environment keys add them to process environment keys | |
// if options contain environment keys add them to process environment keys |
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 code got deleted so no need to fix comment. It is now setup the same as in execFileStreamOutput
Something like this @stevapple?