-
Notifications
You must be signed in to change notification settings - Fork 8
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
Set options with env vars if they're present #261
Conversation
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.
Looks good, thanks @cartermp 👍🏻
I've added a couple of suggestions for handling non-string env vars so we can detect whether the env var was set or not.
if (!options.EnableLocalVisualizations) | ||
{ | ||
options.EnableLocalVisualizations = EnableLocalVisualizations; |
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'm not sure this is correct. If debug was set pragmatically / in settings.json and the env var is set to false, it would still be enabled.
Maybe we could use a bool?
to track if the env var was set or not?
|
||
if (!options.Debug) | ||
{ | ||
options.Debug = Debug; |
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.
Same as above, use a bool?
to detect if the env var was set.
options.Debug = Debug; | ||
} | ||
|
||
if (options.SampleRate == DefaultSampleRate) |
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 think this could use a uint?
too. We need to detect if the env var was set and use whatever value that is.
@MikeGoldsmith good catch, yep. I was trying to be clever here. I think what I have now should do it. |
This should fix #248