solnic / drops

🛠️ Tools for working with data effectively - data contracts using types, schemas, domain validation rules, type-safe casting, and more.
Other
258 stars 7 forks source link

Proposal: Let `:attr` be equivalent to `required(:attr)` in schemas #59

Open zachallaun opened 1 week ago

zachallaun commented 1 week ago

Hi there!

I'd like to propose that bare atoms such as :attr be allowed in schemas and that their behavior be the same as required(:attr). There are three primary reasons I believe this should be the case:

  1. Allows optional attributes to stand out
  2. Improves readability by decreasing clutter
  3. Consistency with Elixir typespecs

Allows optional attributes to stand out

A somewhat unfortunate side-effect of required and optional being the same number of characters is that it can be difficult at-a-glance to pick out which attributes are optional. Consider this example adapted from the README:

defmodule UserContract do
  use Drops.Contract

  schema do
    %{
      required(:user) => %{
        required(:name) => string(:filled?),
        required(:age) => integer(),
        optional(:display_name) => string(),
        required(:address) => %{
          required(:city) => string(:filled?),
          required(:street) => string(:filled?),
          required(:zipcode) => string(:filled?)
        },
        required(:tags) =>
          list(%{
            required(:name) => string(:filled?),
            required(:created_at) => integer()
          })
      }
    }
  end
end

#=>

defmodule UserContract do
  use Drops.Contract

  schema do
    %{
      :user => %{
        :name => string(:filled?),
        :age => integer(),
        optional(:display_name) => string(),
        :address => %{
          :city => string(:filled?),
          :street => string(:filled?),
          :zipcode => string(:filled?)
        },
        :tags =>
          list(%{
            :name => string(:filled?),
            :created_at => integer()
          })
      }
    }
  end
end

While the first version may require careful scanning to see that :display_name is optional, it immediately stands out in the second.

Improves readability by decreasing clutter

Again referencing the above example, I believe the second version is simply easier to read and has a much higher signal-to-noise ratio. And if all of your attributes are required, you get to use the more natural key: value syntax:

defmodule UserContract do
  use Drops.Contract

  schema do
    %{
      user: %{
        name: string(:filled?),
        age: integer(),
        address: %{
          city: string(:filled?),
          street: string(:filled?),
          zipcode: string(:filled?)
        },
        tags:
          list(%{
            name: string(:filled?),
            created_at: integer()
          })
      }
    }
  end
end

Consistency with Elixir typespecs

Typespecs also support required(key) and optional(key) in maps, with bare keys defaulting to required. This is just to say that there is precedence for this behavior and I do not believe it will be surprising to Elixir developers.


If accepted, I'd be happy to take a crack at implementing this! While I have not looked at the internals, I expect that it will not be difficult. (Currently, a function clause error is raised in Drops.Type.Compiler.visit/2 when a bare atom is encountered, so it should be possible to rewrite that case to whatever it expects for required attributes.)

zachallaun commented 4 days ago

For reference, unless there are edge-cases I'm unaware of, implementing this is as simple as:

diff --git a/lib/drops/type/compiler.ex b/lib/drops/type/compiler.ex
index 8a2675e..041dc4d 100644
--- a/lib/drops/type/compiler.ex
+++ b/lib/drops/type/compiler.ex
@@ -32,8 +32,12 @@ defmodule Drops.Type.Compiler do

   def visit(%{} = spec, opts) do
     keys =
-      Enum.map(spec, fn {{presence, name}, type_spec} ->
-        %Key{path: [name], presence: presence, type: visit(type_spec, opts)}
+      Enum.map(spec, fn
+        {name, type_spec} when is_atom(name) ->
+          %Key{path: [name], presence: :required, type: visit(type_spec, opts)}
+
+        {{presence, name}, type_spec} ->
+          %Key{path: [name], presence: presence, type: visit(type_spec, opts)}
       end)

     Map.new(keys, opts)
solnic commented 1 day ago

Oh, I love that. Thanks for bringing this up. How it reads is a bit subjective though, but for me consistency with Elixir's typespecs is a very strong argument here. Let's do this :)