Repository: dashbitco/nimble_options
Pull request #21 (2020-04-07 21:46:31)

Allow complete schema to be passed when generating docs
So as it turns out, `NimbleOptions.docs/1` wouldn't work with "normal" schemas. It only worked with the "special" schema used to validate the schemas themselves. That is, if I called

```elixir
NimbleOptions.docs([producer: [type: :atom]])
```

then it wouldn't generate any docs because `docs/1` only looks for `:keys`. I fixed this but now you can't provide a subsection (through `:doc`) to appear right after `## Options`. This is expected because if my schema is `[producer: [type: :atom]]`, there's nowhere to pass the "main" doc, only options-specific docs. So, I added an optional paramter to `docs/1`. Thoughts @josevalim and @msaraiva?

msaraiva: Hey @whatyouhide!

A schema should always have a typed root node. It can be either a `:keyword_list` or a `non_empty_keyword_list`. We should have a validation that.

So instead of:

```elixir
[producer: [type: :atom]]
```

it should be:

```elixir
[type: :keyword_list, keys: [producer: [type: :atom]]]
```

### Example with docs

```elixir
schema = [
  type: :keyword_list, 
  keys: [
    producer: [type: :atom, doc: "Doc for producer"],
    processor: [type: :atom, doc: "Doc for processor"]
  ]
]

schema |> NimbleOptions.docs() |> IO.puts
```

output:

```
## Options

  * `:producer` - Doc for producer

  * `:processor` - Doc for processor
```

msaraiva: As far as I can remember, the `NimbleOptions.docs()` should add the `type: :keyword_list, keys: ...` automatically. I'm not should why it doesn't. I'll take a look at it. 
whatyouhide: @msaraiva it doesn't, and that's what I'm doing in this PR :)
josevalim: Isn't having a typed root node unnecessary overhead though? I think we can safely assume the root node is always `type: :keyword_list`. We don't need `:non_empty_keyword_list` at this level because you can use `:required` to specify if a subkey is required or not, which would make it non-empty.

So my suggestion is:

1. Consider the given options are always about a keyword list - this means nimble_options can only validate keyword lists

2. Do not automatically generate "## Options"
whatyouhide: This turned out to be a monster to get done correctly 😄, but I believe I fixed everything, so @josevalim @msaraiva it's ready for review :)

Patch:
diff --git a/lib/nimble_options.ex b/lib/nimble_options.ex
index e2d34f7..ebd7f0e 100644
--- a/lib/nimble_options.ex
+++ b/lib/nimble_options.ex
@@ -1,61 +1,59 @@
 defmodule NimbleOptions do
   @options_schema [
-    type: :non_empty_keyword_list,
-    keys: [
-      *: [
-        type: :keyword_list,
+    *: [
+      type: :keyword_list,
+      keys: [
+        type: [
+          type: {:custom, __MODULE__, :type, []},
+          default: :any,
+          doc: "The type of the option item."
+        ],
+        required: [
+          type: :boolean,
+          default: false,
+          doc: "Defines if the option item is required."
+        ],
+        default: [
+          type: :any,
+          doc: "The default value for option item if not specified."
+        ],
         keys: [
-          type: [
-            type: {:custom, __MODULE__, :type, []},
-            default: :any,
-            doc: "The type of the option item."
-          ],
-          required: [
-            type: :boolean,
-            default: false,
-            doc: "Defines if the option item is required."
-          ],
-          default: [
-            type: :any,
-            doc: "The default value for option item if not specified."
-          ],
-          keys: {
-            &__MODULE__.options_schema/0,
-            doc: """
-            Available for types `:keyword_list` and `:non_empty_keyword_list`,
-            it defines which set of keys are accepted for the option item. Use `:*` as
-            the key to allow multiple arbitrary keys.
-            """
-          },
-          deprecated: [
-            type: :string,
-            doc: """
-            Defines a message to indicate that the option item is deprecated. \
-            The message will be displayed as a warning when passing the item.
-            """
-          ],
-          rename_to: [
-            type: :atom,
-            doc: """
-            Renames a option item allowing one to use a normalized name \
-            internally, e.g. rename a deprecated item to the currently accepted name.
-            """
-          ],
-          doc: [
-            type: :string,
-            doc: "The documentation for the option item."
-          ],
-          subsection: [
-            type: :string,
-            doc: "The title of separate subsection of the options' documentation"
-          ]
+          type: :keyword_list,
+          doc: """
+          Available for types `:keyword_list` and `:non_empty_keyword_list`,
+          it defines which set of keys are accepted for the option item. Use `:*` as
+          the key to allow multiple arbitrary keys.
+          """,
+          keys: &__MODULE__.options_schema/0
+        ],
+        deprecated: [
+          type: :string,
+          doc: """
+          Defines a message to indicate that the option item is deprecated. \
+          The message will be displayed as a warning when passing the item.
+          """
+        ],
+        rename_to: [
+          type: :atom,
+          doc: """
+          Renames a option item allowing one to use a normalized name \
+          internally, e.g. rename a deprecated item to the currently accepted name.
+          """
+        ],
+        doc: [
+          type: :string,
+          doc: "The documentation for the option item."
+        ],
+        subsection: [
+          type: :string,
+          doc: "The title of separate subsection of the options' documentation"
         ]
       ]
     ]
   ]
 
   @moduledoc """
-  Provides a standard API to handle keyword list based options.
+  Provides a standard API to handle keyword-list-based options.
 
   `NimbleOptions` allows developers to create schemas using a
   pre-defined set of options and types. The main benefits are:
@@ -64,6 +62,11 @@ defmodule NimbleOptions do
     * Config validation against schemas
     * Automatic doc generation
 
+  ## Schema options
+
+  These are the options supported in a *schema*. They are what
+  defines the validation for the itmes in the given schema.
+
   #{NimbleOptions.Docs.generate(@options_schema)}
 
   ## Types
@@ -187,14 +190,13 @@ defmodule NimbleOptions do
   @spec validate(keyword(), schema()) ::
           {:ok, validated_options :: keyword()} | {:error, reason :: String.t()}
   def validate(options, schema) do
-    case validate_options_with_schema([root: schema], [root: options_schema()], _path = []) do
+    case validate_options_with_schema_and_path(schema, options_schema()) do
       {:ok, _validated_schema} ->
         validate_options_with_schema_and_path(options, schema)
 
-      {:error, message, [:root | path]} ->
+      {:error, message} ->
         raise ArgumentError,
-              "invalid schema given to NimbleOptions.validate/2, in options #{inspect(path)}. " <>
-                "Reason: #{message}"
+              "invalid schema given to NimbleOptions.validate/2. Reason: #{message}"
     end
   end
 
@@ -234,7 +236,13 @@ defmodule NimbleOptions do
     end
   end
 
+  defp validate_options_with_schema(opts, fun, path) when is_function(fun) do
+    validate_options_with_schema(opts, fun.(), path)
+  end
+
   defp validate_options_with_schema(opts, schema, path) do
+    schema = expand_star_to_option_keys(schema, opts)
+
     with :ok <- validate_unknown_options(opts, schema),
          {:ok, options} <- validate_options(schema, opts) do
       {:ok, options}
@@ -297,9 +305,8 @@ defmodule NimbleOptions do
     result =
       with {:ok, value} <- validate_value(opts, key, schema),
            :ok <- validate_type(schema[:type], key, value) do
-        if schema[:keys] do
-          keys = normalize_keys(schema[:keys], value)
-          validate_options_with_schema(value, keys, _path = [key])
+        if nested_schema = schema[:keys] do
+          validate_options_with_schema(value, nested_schema, _path = [key])
         else
           {:ok, value}
         end
@@ -432,7 +439,7 @@ defmodule NimbleOptions do
     is_list(value) && Enum.all?(value, &tagged_tuple?/1)
   end
 
-  defp normalize_keys(keys, opts) do
+  defp expand_star_to_option_keys(keys, opts) do
     case keys[:*] do
       nil ->
         keys
diff --git a/lib/nimble_options/docs.ex b/lib/nimble_options/docs.ex
index b535683..682dd06 100644
--- a/lib/nimble_options/docs.ex
+++ b/lib/nimble_options/docs.ex
@@ -1,35 +1,28 @@
 defmodule NimbleOptions.Docs do
   @moduledoc false
 
-  def generate(schema) do
+  def generate(schema) when is_list(schema) do
     {docs, sections, _level} = build_docs(schema, {[], [], 0})
+    to_string([Enum.reverse(docs), Enum.reverse(sections)])
+  end
 
-    doc = if schema[:doc], do: "#{schema[:doc]}\n\n", else: ""
-    to_string(["## Options\n\n#{doc}", Enum.reverse(docs), Enum.reverse(sections)])
+  # If the schema is a function, we want to not show anything (it's a recursive
+  # function) and "back up" one level since when we got here we already
+  # increased the level by one.
+  defp build_docs(fun, {docs, sections, level}) when is_function(fun) do
+    {docs, sections, level - 1}
   end
 
   defp build_docs(schema, {docs, sections, level} = acc) do
-    cond do
-      schema[:keys][:*] ->
-        build_docs(schema[:keys][:*], acc)
-
-      schema[:keys] ->
-        Enum.reduce(schema[:keys], {docs, sections, level + 1}, &option_doc/2)
-
-      true ->
-        acc
+    if schema[:*] do
+      build_docs(schema[:*][:keys], acc)
+    else
+      Enum.reduce(schema, {docs, sections, level}, &option_doc/2)
     end
   end
 
-  defp build_docs_with_subsection(schema, {docs, sections, level}) do
-    subsection =
-      case schema[:subsection] do
-        nil ->
-          ""
-
-        text ->
-          String.trim_trailing(text, "\n") <> "\n\n"
-      end
+  defp build_docs_with_subsection(subsection, schema, {docs, sections, level}) do
+    subsection = String.trim_trailing(subsection, "\n") <> "\n\n"
 
     {item_docs, sections, _level} = build_docs(schema, {[], sections, 0})
     item_section = [subsection | Enum.reverse(item_docs)]
@@ -37,10 +30,6 @@ defmodule NimbleOptions.Docs do
     {docs, [item_section | sections], level}
   end
 
-  defp option_doc({key, {fun, schema}}, acc) when is_function(fun) do
-    option_doc({key, schema}, acc)
-  end
-
   defp option_doc({key, schema}, {docs, sections, level}) do
     description =
       [get_required_str(schema), get_doc_str(schema), get_default_str(schema)]
@@ -51,13 +40,19 @@ defmodule NimbleOptions.Docs do
       end
 
     indent = String.duplicate("  ", level)
-    doc = indent_doc("* `#{inspect(key)}`#{description}\n\n", indent)
-    acc = {[doc | docs], sections, level}
+    doc = indent_doc("  * `#{inspect(key)}`#{description}\n\n", indent)
 
-    if schema[:subsection] do
-      build_docs_with_subsection(schema, acc)
-    else
-      build_docs(schema, acc)
+    docs = [doc | docs]
+
+    cond do
+      schema[:keys] && schema[:subsection] ->
+        build_docs_with_subsection(schema[:subsection], schema[:keys], {docs, sections, level})
+
+      schema[:keys] ->
+        build_docs(schema[:keys], {docs, sections, level + 1})
+
+      true ->
+        {docs, sections, level}
     end
   end
 


Test patch:
diff --git a/test/nimble_options_test.exs b/test/nimble_options_test.exs
index c2d8f3b..279c75b 100644
--- a/test/nimble_options_test.exs
+++ b/test/nimble_options_test.exs
@@ -25,8 +25,8 @@ defmodule NimbleOptionsTest do
       opts = [stages: 1]
 
       message = """
-      invalid schema given to NimbleOptions.validate/2, in options [:stages]. \
-      Reason: invalid option type :foo.
+      invalid schema given to NimbleOptions.validate/2. \
+      Reason: (in options [:stages]) invalid option type :foo.
 
       Available types: :any, :keyword_list, :non_empty_keyword_list, :atom, \
       :non_neg_integer, :pos_integer, :mfa, :mod_arg, :string, :boolean, :timeout, \
@@ -55,8 +55,9 @@ defmodule NimbleOptionsTest do
       ]
 
       message = """
-      invalid schema given to NimbleOptions.validate/2, in options [:producers, :keys, :*, :keys, :module]. \
-      Reason: unknown options [:unknown_schema_option], \
+      invalid schema given to NimbleOptions.validate/2. \
+      Reason: (in options [:producers, :keys, :*, :keys, :module]) \
+      unknown options [:unknown_schema_option], \
       valid options are: [:type, :required, :default, :keys, \
       :deprecated, :rename_to, :doc, :subsection]\
       """
@@ -751,14 +752,14 @@ defmodule NimbleOptionsTest do
   describe "docs" do
     test "override docs for recursive keys" do
       docs = """
-      ## Options
-
         * `:type` - Required. The type of the option item.
 
         * `:required` - Defines if the option item is required. The default value is `false`.
 
         * `:keys` - Defines which set of keys are accepted.
 
+        * `:default` - The default.
+
       """
 
       assert NimbleOptions.docs(recursive_schema()) == docs
@@ -766,22 +767,19 @@ defmodule NimbleOptionsTest do
 
     test "generate inline indented docs for nested options" do
       schema = [
-        type: :keyword_list,
-        keys: [
-          producer: [
-            type: :non_empty_keyword_list,
-            doc: "The producer. Supported options:",
-            keys: [
-              module: [type: :mod_arg, doc: "The module."],
-              rate_limiting: [
-                type: :non_empty_keyword_list,
-                doc: """
-                A list of options to enable and configure rate limiting. Supported options:
-                """,
-                keys: [
-                  allowed_messages: [type: :pos_integer, doc: "Number of messages per interval."],
-                  interval: [required: true, type: :pos_integer, doc: "The interval."]
-                ]
+        producer: [
+          type: :non_empty_keyword_list,
+          doc: "The producer. Supported options:",
+          keys: [
+            module: [type: :mod_arg, doc: "The module."],
+            rate_limiting: [
+              type: :non_empty_keyword_list,
+              doc: """
+              A list of options to enable and configure rate limiting. Supported options:
+              """,
+              keys: [
+                allowed_messages: [type: :pos_integer, doc: "Number of messages per interval."],
+                interval: [required: true, type: :pos_integer, doc: "The interval."]
               ]
             ]
           ]
@@ -789,8 +787,6 @@ defmodule NimbleOptionsTest do
       ]
 
       docs = """
-      ## Options
-
         * `:producer` - The producer. Supported options:
 
           * `:module` - The module.
@@ -808,33 +804,25 @@ defmodule NimbleOptionsTest do
 
     test "generate subsections for nested options" do
       schema = [
-        type: :keyword_list,
-        doc: "In order to set up the pipeline, use the following options:",
-        keys: [
-          name: [required: true, type: :atom, doc: "The name."],
-          producer: [
-            type: :non_empty_keyword_list,
-            doc: "This is the producer summary. See \"Producers options\" section below.",
-            subsection: """
-            ### Producers options
-
-            The producer options allow users to set up the producer.
-
-            The available options are:
-            """,
-            keys: [
-              module: [type: :mod_arg, doc: "The module."],
-              concurrency: [type: :pos_integer, doc: "The concurrency."]
-            ]
+        name: [required: true, type: :atom, doc: "The name."],
+        producer: [
+          type: :non_empty_keyword_list,
+          doc: "This is the producer summary. See \"Producers options\" section below.",
+          subsection: """
+          ### Producers options
+
+          The producer options allow users to set up the producer.
+
+          The available options are:
+          """,
+          keys: [
+            module: [type: :mod_arg, doc: "The module."],
+            concurrency: [type: :pos_integer, doc: "The concurrency."]
           ]
         ]
       ]
 
       docs = """
-      ## Options
-
-      In order to set up the pipeline, use the following options:
-
         * `:name` - Required. The name.
 
         * `:producer` - This is the producer summary. See "Producers options" section below.
@@ -856,33 +844,28 @@ defmodule NimbleOptionsTest do
 
     test "keep indentation of multiline doc" do
       schema = [
-        type: :keyword_list,
-        keys: [
-          name: [
-            type: :string,
-            doc: """
-            The name.
-
-            This a multiline text.
-
-            Another line.
-            """
-          ],
-          module: [
-            type: :atom,
-            doc: "The module."
-          ]
+        name: [
+          type: :string,
+          doc: """
+          The name.
+
+          This a multiline text.
+
+          Another line.
+          """
+        ],
+        module: [
+          type: :atom,
+          doc: "The module."
         ]
       ]
 
       docs = """
-      ## Options
-
         * `:name` - The name.
 
-        This a multiline text.
+      This a multiline text.
 
-        Another line.
+      Another line.
 
         * `:module` - The module.
 
@@ -914,25 +897,26 @@ defmodule NimbleOptionsTest do
 
   defp recursive_schema() do
     [
-      type: :non_empty_keyword_list,
-      keys: [
-        *: [
-          type: :keyword_list,
+      *: [
+        type: :keyword_list,
+        keys: [
+          type: [
+            type: :atom,
+            required: true,
+            doc: "The type of the option item."
+          ],
+          required: [
+            type: :boolean,
+            default: false,
+            doc: "Defines if the option item is required."
+          ],
           keys: [
-            type: [
-              type: :atom,
-              required: true,
-              doc: "The type of the option item."
-            ],
-            required: [
-              type: :boolean,
-              default: false,
-              doc: "Defines if the option item is required."
-            ],
-            keys: {
-              &recursive_schema/0,
-              doc: "Defines which set of keys are accepted."
-            }
+            type: :keyword_list,
+            doc: "Defines which set of keys are accepted.",
+            keys: &recursive_schema/0
+          ],
+          default: [
+            doc: "The default."
           ]
         ]
       ]