dashbitco / nimble_options

A tiny library for validating and documenting high-level options. 💽
Apache License 2.0
507 stars 38 forks source link

Possible bug: :rename_to should remove the old key #67

Closed axelson closed 3 years ago

axelson commented 3 years ago

The docs for :rename_to state:

Renames a option item allowing one to use a normalized name internally, e.g. rename a deprecated item to the currently accepted name.

Based on a general understanding of what "rename" means, I would expect :rename_to to remove the old key, but it does not. Specifically I'd expect this test to pass:

    test "removes the old key" do
      schema = [
        context: [rename_to: :new_context],
        new_context: [type: {:custom, __MODULE__, :string_to_integer, []}]
      ]

      assert NimbleOptions.validate([context: "1"], schema) == {:ok, [{:new_context, 1}]}
    end

Instead it fails with this error:

  1) test :rename_to removes the old key (NimbleOptionsTest)
     test/nimble_options_test.exs:156
     Assertion with == failed
     code:  assert NimbleOptions.validate([context: "1"], schema) == {:ok, [new_context: 1]}
     left:  {:ok, [{:context, "1"}, {:new_context, 1}]}
     right: {:ok, [new_context: 1]}
     stacktrace:
       test/nimble_options_test.exs:162: (test)

I'm not sure if this can be considered a bug, but this behavior did surprise me.

whatyouhide commented 3 years ago

I think this is a bug, yes. @josevalim can confirm but I’m pretty sure 😉

@axelson any chance you could give a PR with a fix a try? 🙃

josevalim commented 3 years ago

I agree it should remove, yeah. :)

whatyouhide commented 3 years ago

Closing in favor of the PR (#74).