elixir-toniq / vapor

Runtime configuration system for Elixir
MIT License
592 stars 36 forks source link

Defaults don't skip translations #76

Closed QuinnWilton closed 4 years ago

QuinnWilton commented 4 years ago

Given the following app:

defmodule MyApp.Application do
  @moduledoc false

  use Application

  def start(_type, _args) do
    providers = [
      %Vapor.Provider.Env{
        bindings: [
          {:example, "example", default: true}
        ]
      }
    ]

    translations = [
      example: fn s -> String.downcase(s) == "true" end
    ]

    # If values could not be found we raise an exception and halt the boot
    # process
    Vapor.load!(providers, translations)
  end
end

Running the app causes the following crash:

❯ iex -S mix
Erlang/OTP 22 [erts-10.7.1] [source] [64-bit] [smp:16:16] [ds:16:16:10] [async-threads:1] [hipe]

Compiling 6 files (.ex)
Generated my_app app
** (Mix) Could not start application my_app: exited in: MyApp.Application.start(:normal, [])
    ** (EXIT) an exception was raised:
        ** (FunctionClauseError) no function clause matching in String.downcase/2
            (elixir 1.10.2) lib/string.ex:785: String.downcase(true, :default)
            (my_app 0.1.0) lib/my_app/application.ex:16: anonymous fn/1 in MyApp.Application.start/2
            (vapor 0.8.0) lib/vapor.ex:42: Vapor.apply_translation/2
            (elixir 1.10.2) lib/enum.ex:1400: anonymous fn/3 in Enum.map/2
            (stdlib 3.12.1) maps.erl:232: :maps.fold_1/3
            (elixir 1.10.2) lib/enum.ex:2127: Enum.map/2
            (vapor 0.8.0) lib/vapor.ex:16: Vapor.load/2
            (vapor 0.8.0) lib/vapor.ex:31: Vapor.load!/2
            (kernel 6.5.2) application_master.erl:277: :application_master.start_it_old/4
keathley commented 4 years ago

I've been thinking about how a fix for this would work. I'm wondering if we should just drop support for general translations, since each binding now supports a direct translation. When I was originally putting this together we didn't support groups. So it made more sense to do translating after everything had been loaded and flattened. But that seems like it could just be wrong now.

QuinnWilton commented 4 years ago

Now that each binding can take it's own map option, I don't think general translations are needed anymore.

They were also a poor fit because different providers may encode the same option in different ways. i.e. an env file might represent an array as a comma separated string, whereas TOML config will directly represent it as a list. General translations needed to be careful to take into account those differences.