zorbash / opus

A framework for pluggable business logic components
MIT License
354 stars 21 forks source link

Uses a do block for the instrumentation function #7

Closed eloraburns closed 5 years ago

eloraburns commented 5 years ago

This avoids generating dead code that dialyzer complains about.

e.g.

lib/myapp/pipeline.ex:49:guard_fail
Guard test:
is_function((_ -> :ok | {:error, _}), 0)

can never succeed.

Dialyzer is complaining because:

  def definstrument(fun) do
    quote do
      case unquote(fun) do
        f when is_function(f, 0) -> f.()
        f when is_function(f, 1) -> f.(metrics)
      end
    end
  end

where f is a constant for this code. f will either always match the first, or the second, or neither and Dialyzer knows this from compile-time.

I tried to extract the is_function() logic outside of quote, but…there are too many function syntaxes (fn -> … end, fn x -> … end, &f/0, &f/1, &(f(&1))) to cleanly match on. So short of evaluating the AST to see if it results in a function of arity 0 or 1, I'm not sure what else to do.

I did notice that when providing an external module, one implements the underlying instrument/3, and that's basically what this PR does: allow to pass-through the defmacro instrument even inside an Opus.Pipeline.

Thoughts? :)

(And thanks for Opus! It made a chunk of our pipline-style code WAY SIMPLER!

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 71


Totals Coverage Status
Change from base Build 69: 0.0%
Covered Lines: 151
Relevant Lines: 151

💛 - Coveralls
eloraburns commented 5 years ago

Haha, I realized that it's entirely unnecessary to call the instrument macro, as def instrument(a, b, c) doesn't conflict in any way.

The dialyzer issue stands…aside from deprecating and then removing these macros I'm not sure how else you'd fix it though…

zorbash commented 5 years ago

Hey @taavi thanks for taking the time to investigate this, submit this PR and report the dialyzer warning. I'll try to come up with a solution which doesn't involve deprecating the instrument macro but I'm not against deprecating it if we have to. Personally I tend to add standalone instrumentation modules which define instrument/3.