avdi / dotenv_elixir

A port of dotenv to Elixir
MIT License
228 stars 27 forks source link

Allow files to be optional #41

Open fireproofsocks opened 3 years ago

fireproofsocks commented 3 years ago

It would be nice if you could provide a list of optional files (something like how vapor defines a hierarchy of files -- see https://hexdocs.pm/vapor/Vapor.Provider.Dotenv.html#module-file-hierarchy) so you can easily allow users to override default configurations with more specific options. I don't think it's wise to hard-code a list of files that will be looked for -- I think it's a lot nicer (and easier) if you let users supply their own hierarchy of files.

Currently, the following generates an error:

iex(1)> Dotenv.load([".env", ".env.test"])
** (File.Error) could not read file ".env.test": no such file or directory
    (elixir 1.11.1) lib/file.ex:354: File.read!/1
    (dotenv 3.1.0) lib/dotenv.ex:348: Dotenv.read_env_file/1
    (dotenv 3.1.0) lib/dotenv.ex:242: Dotenv.load/1
    (dotenv 3.1.0) lib/dotenv.ex:231: Dotenv.load/1
    (dotenv 3.1.0) lib/dotenv.ex:232: Dotenv.load/1
iex(1)>

I think perhaps the default behavior should be to just quietly ignore a missing file. It would be nice to have an option to enforce that the files exist, e.g. maybe something like:

iex> Dotenv.load([".env", ".env.test"], must_exist?: true)
iloveitaly commented 3 years ago

Maybe a slightly better approach would be to auto-attempt to load .env.test by default when Dotenv.load is called like the ruby library?

I'm a bit hesitant to add more complexity as the usage patterns here are generally pretty consistent.

Outside of .env.test what other use cases were you thinking?

fireproofsocks commented 3 years ago

I would actually advocate for not auto-attempting anything (other than perhaps the namesake .env) -- that should mean less complexity. It should be easier to avoid guessing at a user's needed files and it would avoid a trip to the docs if you just list them explicitly rather than falling back on a pre-defined hierarchical list. It's more common in the .env world (in my experience) to allow files to maybe or maybe not exist. While it's possible to do something like this manually:

Dotenv.load(".env")
if File.exists?(".env.test"), do: Dotenv.load(".env.test")
if File.exists?(".env.test"), do: Dotenv.load(".env.test.local")

or in practice, more likely pivoting off of the config-env, e.g.

# in runtime.exs
import Config
Dotenv.load(".env")
if File.exists?(".env.#{config_env()}"), do: Dotenv.load(".env.#{config_env()}")

But I think it would be more elegant to somehow allow a list of optional files so the loading doesn't stop if a file doesn't exist. I.e. instead of the manual checking, just do:

import Config
Dotenv.load(["one", "two", "three"])

Requiring specific ENV vars to be set makes sense... requiring a specific file to exist does not IMO. One should be able to manually set ENV vars and skip the dot-file altogether, for example. So being able to provide a list of files which may or may not be present would make for a nicer interface IMO. Hope my reasoning makes sense.

iloveitaly commented 3 years ago

I'm not against additional a optional: param to load, I think that makes sense and it's a great addition! Send over a PR if you'd this to be added.

I do think the overall better approach is to pattern off of the ruby dotenv implementation and auto-load a set of files for users (ruby => elixir users will expect this, it uses a convention over configuration preference, and it avoids having the user manage another couple lines of config). That being said, this can be another future change that could be handled separately.