Modules defined in `nofile`

[lasseebert]

Consider this example:

defmodule MyParams do
  use Params.Schema, %{
    nested: %{
      foo: :integer
    }
  }
end

It will define a nested schema in a module called MyParams.Nested.

However, it seems that it will be defined in a "wrong" context. It's source is set to "nofile", which I have had a few problems with:

• Sometimes when I compile my app, it will warn about redefining the nested module
• Code coverage tools does not handle it (errors on missing nofile file).

I have tried to fix it in Params.Def but was not able to get something working.

I think the main problem is that the macro chain is "broken" by some normal functions, which means that the module is actually created on runtime and not compiletime (because the macro expansion stopped).

I see different solutions but was not able to implement them without messing with the current code more than I wanted to:

• Get the __CALLER__ in the original __using__ macro and pass that on to Params.Def. Then create module with something like Module.create(name, quoted_content, Macro.Env.location(caller))
• Use macros all the way down to where the module is created, so that it will be fuilly expanded on compiletime.

[vic]

In my opinion the first option is better, and if I recall correctly (maybe read about it on Metaprogramming Elixir) in general it's better to have a few or a single macros and have it use private functions that produce quoted expressions for it. So yes, we should pass the caller env down the chain. :)

[vic]

Not sure about using Macro.create. Currently the generated code expands to something like:

defmodule MyParams do
  defmodule MyParams.Nested do
     ...
  end
  ...
end

Perhaps it'd be better for the nested module to be defined just like defmodule Nested, maybe that could be causing the 'module redefined' warning?

[lasseebert]

Maybe a good solution would be that the defschema/1 function only created base parameters. Then have another function, e.g. nested_schema_modules we call from __using__(schema) that returns a map of module_name => module_content for the nested params. We could then from the macro create those modules, ensuring that they are created in the right context.

[vic]

Yes, that sounds like a good solution - that's the way I'm doing it at swagger-elixir, basically it generates a list of {module_name, module_content} to be defined, instead of bundling all in a single quoted expression as I did in params.

Feel free to refactor the code as I'm sure it would be a lot cleaner with your suggestions.

[lasseebert]

I'm having a hard time refactoring Def, mainly because there is a number of ways to define a schema, which works in different ways. Would it be a good idea to stick with one way?

There are these ways of defining a schema:

• With defparams
• With a module and use Params.Schema and schema do ... end
• With a module and use Patams.Schema, %{ schema here }

My personal preference is the last one:

defmodule MyParams do
  use Params.Schema, %{
    some_field: :intetger
  }
end

This is the least "magic" (compared to defparams) and gives the possibility to define changeset functions to apply e.g. custom validation.

defparams is kind of magical since it defines a module under Params namespace, so one would have to know not to define schemas with the same name in different modules, And schema do seems to just be another way of making an Ecto.Schema where we can't use the Params syntax for nested schemas (right?).

Would that be a too big change to remove all but one way of creating a schema and in that way make the code less complex?

[lasseebert]

It is actually only defparams I want to remove. After more code review, the schema do-way is good :)

[lasseebert]

Ok, now I have a working solution which have not messed with the public API. defparams still works ;)

• Reimplemented defschema to create modules in the right context
• Reimplemented defparams to use defschema.

[lasseebert]

Will make a PR when I have cleaned up the code a bit

[vic]

Hey @lasseebert sorry for the late response been a bit busy with life/work. I'm reading your PR, thanks !