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

add validate/2, load/2, and dump/2 function generation to nested schema modules #63

Closed
wants to merge 1 commit into from
Closed

add validate/2, load/2, and dump/2 function generation to nested schema modules #63

wants to merge 1 commit into from

Conversation

punchcafe
Copy link

Hello!

This is a Pull Request to add some small Quality of Life improvements to the nested schema module generation.

@phcurado
Copy link
Owner

phcurado commented Feb 23, 2024

@punchcafe thanks for the PR. Could you elaborate what problem you are facing that requires to have these functions on the nested module?
The PR looks good tho, needs formatting which was flagged on the CI.

@punchcafe
Copy link
Author

Thank you! I've updated that now, so hopefully it should be green!

@phcurado
Copy link
Owner

thanks! Could you elaborate what problem you are facing that requires to have these functions on the nested module?
I just want to understand your problem and see if this is the best solution.

@punchcafe
Copy link
Author

Sure! It was mostly just a matter of ergonomics. Having to call the main param each time with the Module felt a bit verbose, so we were wondering if we could make it more succinct by using the GenServer kind of approach.

@phcurado
Copy link
Owner

I don't see much gain on using

MyParam.load(%{})
# instead of 
Parameter.load(MyParam, %{})

I think the second approach is more explicit that we are using the parameter library to load parameters from your module. The first approach we are injecting a function which will be hidden in the nested macro. Also when you mention the GenServer, seems Parameter is consistent with their implementation: https://hexdocs.pm/elixir/1.12/GenServer.html
Example:

{:ok, _} = GenServer.start_link(Stack, [:hello], name: MyStack)
GenServer.call(MyStack, :pop)

So on genserver, same as parameter, you have to use the module for calling the module that implements the behaviour. The documentation also mention that we can wrap the gen server functions (example in the Client/Server API). I would recommend the same for parameter:

defmodule MyParam do
  use Parameter.Schema

  param do
    field :my_param, :string
  end

  def load(inputs, opts \\ []) do
    Parameter.load(__MODULE__, inputs, opts)
  end
end

Now if you still want that all your param modules to have these injected functions, I recommend doing the following:

defmodule MyApp.Param do
  defmacro param(module, block) do
    quote do
      import Parameter.Schema, except: [param: 2]

      Parameter.Schema.param unquote(module) do
        unquote(block)

        def load(params, opts \\ []) do
          Parameter.load(__MODULE__, params, opts)
        end

        def dump(struct) do
          Parameter.dump(__MODULE__, struct)
        end
      end
    end
  end
end

## Then you can use your own parameter version in your modules
defmodule MyApp.UserController do
  import MyApp.Param

  param UserParam do
    field :first_name, :string
  end
end

Now if we test this out:

iex> MyApp.UserController.UserParam.load(%{"first_name" => "Name"})
...> {:ok, %{first_name: "Name"}}

This way you can also extend the param/1 macro to inject these functions instead of only the nested part param/2.

For me this approach is more clear that you are injecting these functions into your nested module, you can even extend to offer more capabilities like forcing every load to be a struct and so on.

@punchcafe
Copy link
Author

Thanks for your feedback. I see your concerns about adding this as it brings in implicit function declarations.

I appreciate the solutions you've offered above but these don't really meet our use case, as our priority is to be able to declaratively define request schemas with minimal boiler plate and no extra internal macros.

Thank you for your time!

@punchcafe punchcafe closed this Feb 27, 2024
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 this pull request may close these issues.

2 participants