Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
zachallaun opened this issue Nov 22, 2024 · 2 comments · May be fixed by #60
Open

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

zachallaun opened this issue Nov 22, 2024 · 2 comments · May be fixed by #60

Comments

@zachallaun
Copy link

zachallaun commented Nov 22, 2024

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
Copy link
Author

zachallaun commented Nov 25, 2024

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
Copy link
Owner

solnic commented Nov 28, 2024

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 :)

@zachallaun zachallaun linked a pull request Dec 10, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants