Skip to content
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

Add support for a default value in System.get_env/1 #3798

Merged
merged 3 commits into from Mar 17, 2019

Conversation

Projects
None yet
6 participants
@guilleiguaran
Copy link
Contributor

commented Sep 29, 2015

Similarly to Erlang os:getenv/2 this allow to return a default value in System.get_env when the environment variable is undefined.

@ryngonzalez

This comment has been minimized.

Copy link

commented Sep 29, 2015

LGTM 👍

@josevalim

This comment has been minimized.

Copy link
Member

commented Sep 29, 2015

Because System.get_env returns a binary or nil, we can use || default and it will never be ambiguous. That said, I don't think we really need this feature. :) Thank you!

@josevalim

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

@guilleiguaran can you please rebase?

@guilleiguaran guilleiguaran force-pushed the guilleiguaran:system-get-env-2 branch from 0de8abd to 2c2c694 Mar 16, 2019

@guilleiguaran

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

@josevalim rebased!

@fertapric

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

System.get_env/2 returns either a binary or nil, should the default value follow that contract too?

@josevalim

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

@fertapric good call. Let's keep it as String.t | nil and add a guard. /cc @guilleiguaran

@spencerdcarlson

This comment has been minimized.

Copy link

commented Mar 16, 2019

@guilleiguaran & @josevalim,

Personally I would really like to see a case for when is_integer(default) that attempts to cast the value, returns integer() and handles any exceptions. This would prevent the Config file from ever blowing up if I need an actual integer

   def get_env(varname, default) when is_integer(default) do
     case :os.getenv(String.to_charlist(varname)) do
       false -> default
       other -> other |> List.to_string() |> String.to_integer()
     end
   rescue
     _ -> default
   end

Just yesterday we faced this issue where our app kept dying because the env value was "10" when it needed to be 10

config app:
  pool_size: System.get_env("POOL_SIZE") || 10 

When POOL_SIZE=10 it blows up because get_env/1 returns a string
and I forgot to wrap it in String.to_integer(). Wrapping it is not a great solution
because if the POOL_SIZE= or any value that can not be cast to an integer, it will still blow up.
We end up writing helpers:

config app:
  pool_size: Utilities.get_env_integer("POOL_SIZE", 10),

@guilleiguaran guilleiguaran force-pushed the guilleiguaran:system-get-env-2 branch from 2c2c694 to 07b2c94 Mar 16, 2019

@guilleiguaran

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

@fertapric @josevalim changed the spec back to String.t | nil and added guard.

`varname` is a string, or `nil` if the environment
variable is not set.
`varname` is a string. If the environment variable
is undefined, returns default (or `nil` if not provided)

This comment has been minimized.

Copy link
@fertapric

fertapric Mar 16, 2019

Member

Minor nitpick: use "not set" instead of "undefined" for not un-doing previous efforts #8851

Suggested change
is undefined, returns default (or `nil` if not provided)
is not set, returns `default` (which is `nil` unless specified
otherwise).

This comment has been minimized.

Copy link
@whatyouhide

whatyouhide Mar 16, 2019

Member

We should also add in the documentation as well that the default needs to be a binary.

This comment has been minimized.

Copy link
@guilleiguaran

guilleiguaran Mar 16, 2019

Author Contributor

wdyt:

The returned value of the environment variable
varname is a string. If the environment variable
is not set, returns the string specified in default or
nil if not specified.

This comment has been minimized.

Copy link
@whatyouhide

whatyouhide Mar 16, 2019

Member

I'm not sure what you mean @guilleiguaran, I'm sorry. I'm just saying that we are requiring the default argument to be either nil or a string but we're not documenting that if present, the default argument should be a string.

This comment has been minimized.

Copy link
@guilleiguaran

guilleiguaran Mar 16, 2019

Author Contributor

@whatyouhide oops sorry, the format of the comment was wrong:

The returned value of the environment variable
varname is a string. If the environment variable
is not set, returns the string specified in default or
nil if not specified.

(maybe none is specified sounds better than not specified?)

@whatyouhide whatyouhide changed the title Add support for a default value in System.get_env Add support for a default value in System.get_env/1 Mar 16, 2019

@whatyouhide

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

If we add the default as a default argument to the already existing System.get_env/1, then we cannot use @doc since: "1.9.0" in System.get_env/2. Will this be a problem?

@josevalim

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

@guilleiguaran

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

@spencerdcarlson I personally liked allowing any as default value since that's the know behavior of others langs with os.getenv including Erlang's os:getenv but I understand the reasoning of @fertapric and @josevalim to keep it consistent as string in any case, also it might be less surprising since that is how it is returned by underlying OS.

@spencerdcarlson

This comment has been minimized.

Copy link

commented Mar 16, 2019

@josevalim, @whatyouhide, @fertapric what are your thoughts on adding another case for a default integer? - #3798 (comment)

@whatyouhide

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

what are your thoughts on adding another case for a default integer? - #3798 (comment)

@spencerdcarlson I am strongly against this since I am convinced it would be surprising to users that expect a system environment variable to be a string and a string only. For example, what do we do if the value is an integer surrounded by whitespace? Wouldn't that be an integer anyways? Decisions like this are not the job of System.get_env/1 but of the user of this function. The user is in charge of calling String.to_integer/1 or Integer.parse/1 or whatever is more appropriate.

I personally liked allowing any as default value since that's the know behavior of others langs

@guilleiguaran imagine I store the database port in an environment variable, and I want to have a default. If we allow any defaults, I might do this:

System.get_env("DATABASE_PORT", 5432)

but then this gives me an integer as the default value but a string if the variable is set. If we only allow string default values, I can safely do:

System.get_env("DATABASE_PORT", "5432") |> String.to_integer()

and treat the default value the same as I would treat the value if it was set.

@whatyouhide

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

@josevalim what are your thoughts on adding:

# This is the one we have today
def get_env(var), do: get_env(var, nil)

@doc since: "1.9.0"
def get_env(var, default) when is_binary(var) and is_binary(default) do
  # code that we have today with default handling
end

This would solve the @doc since problem and also enforce the user-passed default to be a binary instead of incidentally allowing nil since that's the default value?

@guilleiguaran

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

@whatyouhide

re: integer, agree 100%, that were exactly my thoughts when @josevalim and @fertapric asked me to keep the old contract.

re: making get_env/2 and get_env/1 different functions, agree! but I'm going to wait to @josevalim opinion before of updating the code.

@fertapric

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

@whatyouhide @guilleiguaran there are other places in the codebase in the same situation. For example, Date.new(year, month, day) was introduced in 1.3.0 and changed to Date.new(year, month, day, calendar \\ Calendar.ISO) in 1.5.0. So, Date.new/3 would be @doc since: "1.3.0" and Date.new/4 would be @doc since: "1.5.0".

Writing two separate functions would allow to specify two different @doc since:, but it seems like a lot of work just to get the @doc since: metadata right. On top of that, it might slightly hurt documentation since you would have two separate @docs and two different functions in IEx and at the sidebar in ExDoc.

Said that, I would remove the @doc since: "1.9.0" (or, in case a version is required, use the version in which get_env/1 was introduced).

@spencerdcarlson

This comment has been minimized.

Copy link

commented Mar 16, 2019

System.get_env("DATABASE_PORT", 5432)

It would be odd to assume that System.get_env would convert the default value into a string.

The value and type of the default should be respected. There is no perfect solution here, but adding another case simply provides another convenience function, which reduces boilerplate and error handling.

@spencerdcarlson

This comment has been minimized.

Copy link

commented Mar 16, 2019

For example, what do we do if the value is an integer surrounded by whitespace?

I think this is an extreme edge case that shouldn't impact the decision. It would also be handled by a rescue clause that simply returns the default. I believe the only way to get an integer surrounded by whitespace is:
1.

System.put_env("SAMPLE", " 10")
System.get_env("SAMPLE", 11)
# => 11
System.get_env("SAMPLE", " 10")
# => " 10"
# this wouldn't try and cast

I believe most shells trim whitespace when there are no quotes

export SAMPLE=10\s\s
iex> System.get_env("SAMPLE", 11)
# => 10

# This will have whitespace but to me is a typo and you should get the default value
export SAMPLE="10 "
iex> System.get_env("SAMPLE", 11)
# => 11
# If it isn't a typo and you need "10 "
iex> System.get_env("SAMPLE", "11 ")
iex> System.get_env("SAMPLE")
# => "10 "
@whatyouhide

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

@spencerdcarlson you can go back to my database port example to find why I am strongly against automatic casting of strings to integers. If you set the variable to a proper integer then everything's fine, but if you set it to "5432 " (with a space) then the casting silently fails and returns a string and now you can't expect it to be a string or an integer. I think we should go with a consistent interface where the function only returns strings and it's the user responsibility for casting them to whatever. At least for me, in order to consider this I would need reaserch backing this idea up, such as other languages that do it and practical use cases (with code) that show that this is useful 🙃

@guilleiguaran guilleiguaran force-pushed the guilleiguaran:system-get-env-2 branch from f65103c to bcea182 Mar 16, 2019

@guilleiguaran guilleiguaran force-pushed the guilleiguaran:system-get-env-2 branch from bcea182 to 5ea557c Mar 16, 2019

@josevalim josevalim merged commit ecf36ad into elixir-lang:master Mar 17, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@josevalim

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

❤️ 💚 💙 💛 💜

@guilleiguaran guilleiguaran deleted the guilleiguaran:system-get-env-2 branch Mar 17, 2019

Hanspagh added a commit to Hanspagh/elixir that referenced this pull request Mar 19, 2019

laurinenas added a commit to laurinenas/elixir that referenced this pull request Apr 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.