Skip to content

convert inject/1 to a macro #7

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.

Sign up for GitHub

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

Open
levine1726 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

convert inject/1 to a macro #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
50fbd24
convert inject/1 to a macro
levine1726 Sep 28, 2022
5572580
update test to evaluate code with update env vars
levine1726 Oct 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions lib/inject.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,25 @@ defmodule Inject do
:ok
end

defmacro inject_module(mod) do
if Application.get_env(:inject, :disabled) do
mod
Copy link
Author

@levine1726 levine1726 Oct 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I didn't wrap this in a quote block. I can do it if needed, but I think it would be redundant since we're not modifying the incoming atom or AST (not that I would see inject/1 receiving an AST any time soon).

Copy link
Author

@levine1726 levine1726 Oct 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, doing quote(do: mod) here throws warnings and I'm not sure why.... 🤔

Copy link

@teamon teamon Oct 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since quote is kinda like literal copy paste, what would be the result of such code?

else
quote bind_quoted: [mod: mod] do
Inject.Registry
|> Registry.lookup(mod)
|> Enum.reverse()
|> find_override() || mod
end
end
end
Copy link

@teamon teamon Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best way to visualise macros (well, for me at least) is to think of them as copy & paste.
With macro defined like this we would get this code:

def fancy do
  i(Dep).fun()
end

replaced with

def fancy do
  (
      if Application.get_env(:inject, :disabled) do
         mod
       else
         Inject.Registry
         |> Registry.lookup(mod)
         |> Enum.reverse()
         |> find_override() || mod
       end
  ).fun()
end

This would still call the Application.get_env every time fancy/0 is called.

What we want instead, is to evaluate the Application.get_env call in compile time, inside macro:

defmacro inject(mod) do
  # run stuff here, during compilation
  if letsdothis do
    quote(do: injectstuff)
  else
    quote(do: nah)
end

This way the body of fancy/0 would be either (injectsutff).fun() or (nah.fun()).

Now, what should injectsutff and nah be is not for me to decide ;)
But I'll give you one more hint - it's usually a good practice to put the least amount of code inside quote as possible. If you can replace some lines with a function call and just call that function inside quote it should be done.

Can't wait to see the v2! 🎸

Copy link
Author

@levine1726 levine1726 Oct 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a stab at V2. I just amended the commit; should have just squashed down before merging so you could see the change history but oh well, at least the change is small! I also asked a separate question about mod which you can see in the diff.

By the way, the test in test/inject_configuration_test.exs now fails, since changing the environment at runtime won't change the compiled code. Is there a way to recompile the code just for a test, or should we just remove this check? For what it's worth, I don't know if it's possible to do this from a quick google test.


def i(mod) do
inject(mod)
end

def inject(mod) do
if Application.get_env(:inject, :disabled) do
mod
else
Inject.Registry
|> Registry.lookup(mod)
|> Enum.reverse()
|> find_override() || mod
end
inject_module(mod)
Copy link

@teamon teamon Oct 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there :)

Calling inject_module macro here would run the macro once during compilation and that's it. What we want is:

  • An inject_module function (not macro) that does the injection (always, unconditionally)
  • An inject macro, that based on application env config either does nothing or calls that inject_module function.

end

defp find_override([]), do: nil
Expand Down
2 changes: 1 addition & 1 deletion test/inject_configuration_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ defmodule InjectConfigurationTest do
test "it does not lookup modules in the registry" do
register(ExampleModule, StubModule)
Application.put_env(:inject, :disabled, true)
assert "unstubbed" = i(ExampleModule).hello()
assert {"unstubbed", []} = Code.eval_quoted(quote(do: inject_module(ExampleModule).hello()))
Application.put_env(:inject, :disabled, false)
end
end
Expand Down