-
Notifications
You must be signed in to change notification settings - Fork 47
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
Environment Parameters #209
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, just the edge case I think should be handled differently
|
||
def resolve(value) | ||
environment_variable = ENV[value] | ||
raise ArgumentError, "The environment variable #{value} is not set" if environment_variable.to_s.empty? |
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 should be if environment_variable.nil?
because an empty parameter might be perfectly legitimate and ENV
returns nil if the environment variable is not set
irb(main):007:0> ENV["FOO"]
=> nil
irb(main):008:0> exit
patrickrobinson:$ export FOO=""
patrickrobinson:$ irb
irb(main):001:0> ENV["FOO"]
=> ""
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.
Feels kind of odd.
What's the context where nil
is undesirable but ""
string might be legitimate? Do you have an example of how you might use this?
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.
nil
is only possible if you do not set the Environment variable. Any empty string is only possible if you explicitly set it to an empty string. In that case we should trust the user knows what they are doing and did this on purpose.
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.
@stevehodgkiss what are you're thoughts on this. Should it be allowed?
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 an empty string is a valid input we should support. We could check ENV.key?(value)
or environment_variable.nil?
as @patrobinson mentioned.
end | ||
|
||
end | ||
""" |
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.
Indentation is off...
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.
Thanks!
This completes GH-170