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

New and improved executor #390

Merged
merged 5 commits into from
Mar 16, 2016
Merged

New and improved executor #390

merged 5 commits into from
Mar 16, 2016

Conversation

christophermaier
Copy link
Collaborator

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

{: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
Copy link
Member

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

Copy link
Member

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.

@vanstee
Copy link
Member

vanstee commented Mar 15, 2016

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.

@kevsmith
Copy link
Member

💯 @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
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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!

christophermaier and others added 4 commits March 16, 2016 12:31
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.
@christophermaier christophermaier force-pushed the cm/new-executor branch 2 times, most recently from 9952794 to e1639ce Compare March 16, 2016 17:42
christophermaier added a commit that referenced this pull request Mar 16, 2016
@christophermaier christophermaier merged commit 75ffdd5 into master Mar 16, 2016
@christophermaier christophermaier deleted the cm/new-executor branch March 16, 2016 17:58
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.

6 participants