-
Notifications
You must be signed in to change notification settings - Fork 71
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
New and improved executor #390
Conversation
{:ok, defs} -> | ||
case interpret(defs, raw, [], %{}) do | ||
def initialize(%Ast.Invocation{args: args, meta: command_model}=invocation) do | ||
case Enum.reduce(command_model.options, %{}, &(prepare_option(&1, &2))) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to wrap prepare_option
in an anonymous function. Could just use &prepare_option/2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a good place to use with
defs = Enum.reduce(command_model.options, %{}, &(prepare_option(&1, &2)))
with {:ok, options, args} <- interpret(defs, args, [], %{}),
:ok <- check_required_options(defs, options) do
options = set_defaults(invocation, options)
{:ok, options, args}
end
Also not sure if defs
can ever not be a map so that first case might be unnecessary.
The only two important comments were the ones about creating a request struct and whether or not we're rendering a template per item in the output list. Other than that this looks great. Seriously good work @christophermaier. |
If we can all agree the new template rendering logic is correct I'm 👍 to merge. Let's table the request/response struct to a new ticket. |
:not_found | ||
{:ambiguous, names} -> | ||
{:ambiguous, names} | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following how the anonymous func is getting the namespace and command name from the inputs passed. Maybe more comments about how this works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Piper invokes the anonymous function with the namespace and command name @lhaskins. If that helps at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lhaskins command_resolver_fn
returns a function that closes over the passed in user. This function is passed into the parser and is used as a callback to lookup the relevant information; the parser determines the bundle name and command name to be passed in based on the parsed text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! gotcha - a callback function that gets executed in Piper. Thanks!
Uses the new parser and simplifies A LOT of what the executor does. Introduces a new concept of an "execution plan" for pipeline stages, in an attempt to decouple from the parser AST (though that's not quite complete just yet). Lots more remains to be done, but this takes care of a huge chunk of the work.
9952794
to
e1639ce
Compare
e1639ce
to
5b857ea
Compare
New and improved executor
Uses the new parser and simplifies A LOT of what the executor does.
Introduces a new concept of an "execution plan" for pipeline stages,
in an attempt to decouple from the parser AST (though that's not quite
complete just yet).
Lots more remains to be done, but this takes care of a huge chunk of
the work.
Closes #286