slogsdon / mandrill-elixir

a Mandrill wrapper for Elixir
MIT License
51 stars 13 forks source link

Mandrill key should not have to be an environment variable #8

Open RobinClowers opened 8 years ago

RobinClowers commented 8 years ago

Requiring the key to be set as in env does not seem to be idiomatic for elixir apps, it would be great if you could use the mix config system. You could still default the value what's in env, but fall back to using the config value defined by mix.

slogsdon commented 8 years ago

@RobinClowers I believe the documentation just needs to be updated. This was previously added: https://github.com/slogsdon/mandrill-elixir/blob/master/lib/mandrill.ex#L49-L52. It pulls the key from Application environment variable, but if nil, it reaches out to System environment variable.

As a note, this should probably be updated as well to remove the || operator, but it should work as is.

RobinClowers commented 8 years ago

That's great news, I'll send you a pr to update the docs this weekend.

RobinClowers commented 8 years ago

When you say remove the || operator, you mean remove the option to set it through the environment variable? I still think that's a nice fallback, in some cases it's more convenient to set it that way.

slogsdon commented 8 years ago

@RobinClowers, Here's what I meant by that statement. Right now this is essentially what the code looks like:

Application.get_env(:mandrill, :key) || System.get_env("MANDRILL_KEY")

When I said that the || operator should probably be removed, I was suggesting that the use of that operator is unnecessary since Application.get_env/3 is a better alternative. Using that, the above code would look something like this:

Application.get_env(:mandrill, :key, System.get_env("MANDRILL_KEY")

Looking back, I could have been clearer on that, so I apologize for any confusion.

RobinClowers commented 8 years ago

Awesome, thanks for the explanation, that makes total sense.