Harmonise Editor, Generator, Executor and Handler TypeScript APIs

[kipz]

No description provided.

[kipz]

Example of current API:

https://github.com/atomisthqa/handlers/blob/d049285eacd9c3ab018ee60cb59913c70d5ca60b/.atomist/handlers/IssueHandler.ts

Issues:

• use of atomist/Atomist top level var/interface is too broad. Perhaps something more specific, like model etc.
• use of var injection can be dangerous and magical - i.e we might be overwriting something
• Very different from Generator/Editor/Executor programming models
• not obvious how we might share & reuse common path expressions

Benefits:

• Very terse
• Currently working!

[kipz]

Current proposal:

//The below could be shared/lookup/created in different ways...
class ClosedIssues implements PathExpression<TreeNode, TreeNode> {
  expression: string = "/issue[.state()='closed']"
}


let handler: Handler<TreeNode,TreeNode> = {
  query: new ClosedIssues(),//could be some sort of lookup rather than new..
  handle (match: ContextMatch<TreeNode,TreeNode>){
    let issue = match.root()
    let message = match.messageBuilder().regarding(issue)
    message.withAction(message.actionRegistry().findByName("AddLicense"))
    message.withAction(message.actionRegistry().findByName("HelloWorld"))
    return message; //we could allow message or messages here..
  }
}

[jessitron-stripe]

it returns the message <3 <3

[jessitron-stripe]

It still has atomist as a magic global var? If it's going to return data, it could take atomist as a parameter too?

[kipz]

Oh yeah, updated.

[kipz]

Might be more consistent to have a context() function as below?

let handler: Handler<TreeNode,TreeNode> = {
  query: new ClosedIssues(),//could be some sort of lookup rather than new..
  handle (match: ContextMatch<TreeNode,TreeNode>){
    let issue = match.root()
    let message = match.context().messageBuilder().regarding(issue)
    message.withAction(message..context().actionRegistry().findByName("AddLicense"))
    message.withAction(message..context().actionRegistry().findByName("HelloWorld"))
    return message; //we could allow message or messages here..
  }
}

[slimslenderslacks]

1. So "runner" of these handlers knows to send the message or messages?
2. There is certainly an advantage to having the lexical scope for a handler definition set up by either the test or the "runner". I do miss just how terse the previous expression is though. What about a version of handle that supports an inline path expression if we don't specify a query property?

[kipz]

As per @jessitron, perhaps this would simplify further:

let handler: Handler<TreeNode,TreeNode> = {
  query: new ClosedIssues(),//could be some sort of lookup rather than new..
  handle (match: ContextMatch<TreeNode,TreeNode>){
    let issue = match.root()
    return new MessageBuilder().regarding(issue).withAction("AddLicense").withAction("HelloWorld)
  }
}

[jessitron-stripe]

I love the part where the handler can return data about what to do, and its runner can take those actions. That makes it so testable! and easier to troubleshoot.

[slimslenderslacks]

So would the handle function return an Array<Actionable> or something? That would work for Messages which really do only have one Actionable thing that you can do with them (send). But we're going to have to be explicit about ordering if a handler generates a list of Actionable things.

[kipz]

The only collections are arrays, but we could support returning a single message or an array of them.

1. So "runner" of these handlers knows to send the message or messages?

yep.

2. There is certainly an advantage to having the lexical scope for a handler definition set up by either > the test or the "runner". I do miss just how terse the previous expression is though. What about a version of handle that supports an inline path expression if we don't specify a query property?

I think the only way to support an inline expression is to have a concrete var to operator on, which implies a magic var....

[kipz]

With more concrete match:

let handler: Handler = {
  query: "/issue[.state()='closed']",
  handle (match: MatchedIssue){
    return new MessageBuilder(match.root())
      .withAction("AddLicense")
      .withAction("HelloWorld");
  }
}

[jessitron-stripe]

pretty!

[jessitron-stripe]

@slimslenderslacks if you want to say "do this and then this and then this", then you add a method to the builder. send() could be followed by andThen(anotherThingToDo)

[slimslenderslacks]

handlers are not only for sending Messages ... so when you are collecting an Array of Actionable things, it doesn't mean just sendable. So I could see a Message being something Actionable (because you can send it) but the handle would not be typed as returning Array<Message>

1. would there be an import of the Handler type in this file? If so, what is the type of handle function?
2. do modules make sense here?

declare module "Atomist" {
    // register handler
    export function on(path: string): Handler
    // or
    export function on( exp: PathExpression): Handler
}

so that you could write something like:

on("/build").handle(match: MatchedIssue) {
   return new MessageBuilder(match.root()).say("something")
}

[kipz]

handlers are not only for sending Messages ... so when you are collecting an Array of Actionable things, it doesn't mean just sendable. So I could see a Message being something Actionable (because you can send it) but the handle would not be typed as returning Array

I'm struggling to understand what you're saying above exactly. In my example, we are returning a MessageBuidler (which perhaps needs a better name), but the overall thinking is that it's just data, and the framework (rug-runner) knows how to dispatch messages and take any actions.

1. would there be an import of the Handler type in this file? If so, what is the type of handle function?

Yep. So TS supports union types, so the type could include any other Actionables you can imagine.

2. do modules make sense here?

I think modules themselves are orthogonal, but we could export a function. However, we wouldn't want that to do any JVM interop - which basically gives us the same security, testability and magic problems.

[kipz]

So we could expose a function called on and when called, it generates an implementation of the Handler interface, but we'd still need to bind it to a var (we can't define a global var from function scope I think)

let handler = on<TreeNode,TreeNode>("/issue[.state()='closed']", match => {
      return new MessageBuilder()
        .regarding(match.root())
        .withAction("AddLicense")
        .withAction("HelloWorld");
})

So users could choose which one to implement.

Is that acceptably terse? cc @johnsonr @slimslenderslacks

[Lawouach]

I appreciate the idea of chainable actions, that seems to be inline with the way most JS I've seen tend to work as well. That's nice to look idiomatic (even though in typescript, this may not be?)

[johnsonr]

When you say users could choose which one to implement which options do you mean?

I like the last example in this thread, at this point.

[kipz]

What I mean is that these two are equivalent and supported under the proposal:

let handler: Handler = {
  query: "/issue[.state()='closed']",
  handle (match: MatchedIssue){
    return new MessageBuilder(match.root())
      .withAction("AddLicense")
      .withAction("HelloWorld");
  }
}

let handler2 = on<MatchedIssue>("/issue[.state()='closed']", match => {
      return new MessageBuilder()
        .regarding(match.root())
        .withAction("AddLicense")
        .withAction("HelloWorld");
})

Because the on function in the second example is a bit of TypeScript that emits an implementation of Handler.

[johnsonr]

I like the approach. I would probably use the second style, but it makes total sense to map it onto the first as the fundamental thing. So I'm good with where we are here.

[slimslenderslacks]

1. it'll be difficult for a handler author to see when they'd need a parameterized type other than TreeNode,TreeNode. We should be able to craft a version of the on function that uses the match type to do type inference. The former expression is nicer in that regard, but I assume that the Handler is typed for a root TreeNode and TreeNode matches so the generics in the second expression are hard to justify if they're not really parameters
2. the var has to be there because we have to leave some global var behind (containing our new Handler), but couldn't on just register that Handler into a single global registry var? Why do we have to "let" that in to the global scope again?

[johnsonr]

1. Maybe I don't understand your point. I think it will often be valuable to have a strongly typed expression.

[kipz]

@slimslenderslacks If we're going to get rid of the use of let to expose the var for introspection by Rug, then I feel we should do it for all the other Rugs too (editors, generators and executors).

A replacement would be to expose a register function that manipulates a global var to register Rugs. The on function above could call this to hide it from the terse version.

I think we need to make a call on this before moving too much further forward with the design.

[ddgenome]

In looking at tree expressions for editors/projects vs. handlers/events, we may to have an inconsistency. The tree expression for an editor has an implicit project node at its root: /src/main/java. The root node, represented by /, is of type Project().

The tree expression for a handler seems to have the root node explicitly: /Issue()[@state='closed'], because then we call match.root() to get an Issue object. Do others see it this way?

If so, perhaps we have things backwards. Instead of

let handler2 = on<MatchedIssue>("/Issue()[@state='closed']", match => {
      return new MessageBuilder()
        .regarding(match.root())
        .withAction("AddLicense")
        .withAction("HelloWorld");
})

it should be

let handler2 = on<MatchedEvent>("/Issue()[@state='closed']", root => {
      return new MessageBuilder()
        .regarding(root.match()) // <= swapped
        .withAction("AddLicense")
        .withAction("HelloWorld");
})

where the type of root is an Event() and the return value of match() is the first explicit step of the tree expression, Issue() in this case.

[kipz]

Makes sense to me @ddgenome. @rod?

[Lawouach]

I don't know. I see what you mean @ddgenome but at the same time the interface does say on<MatchedIssue> so it seems fair to get a match variable (or whatever this is called in typescript).

I think we shouldn't be b e mislead by the fact that this specific handler wants to act on the issue itself.

More generally, we'll want to act on a node but that won't always be the root node. In XPath at least, one expect to be able to play on any node the expression matches not the root node of the tree. Put differently, I assume we could have something like this:

let handler2 = on<MatchedIssue>("/dcvs/github.com/org-name/project-name/Issue()[@state='closed']", match => {
return new MessageBuilder()
        .regarding(match.node())
        .withAction("AddLicense")
        .withAction("HelloWorld");
})

where the we still act on the Issue node which isn't the root.

[jessitron]

From a meeting with @cd and @kipz and @slimslenderslacks today:

All commands will (eventually) be asynchronous.
Jess:
So people can’t react to the response (fail/succeed) in the program. They could pass instructions (data) about what to do then
And don’t try/catch because that exception is not coming to you… so in the proxy (or nearby) we should catch exceptions and record them and then take the onFail action if they assign one
For Jan 18th, we are going with the current implementation.

Outstanding question:
Long-term, do we want to present an imperative or functional face?
Either way, only data can be passed over a queue to the asyc execution. Even if we present an imperative face, it’ll be commands-as-data in the background eventually.

We also talked about whether we want to add methods to treeNodes or supply TypeScript functions that accept treeNodes. (Either can return/record commands)
Putting the method on the tree node makes it discoverable. Also,
Most of these are going to be implemented by us.
We can implement a general http-call one, which can be extended in TypeScript. Customers won’t typically need to write Java. But we could, for discoverability. And we can, to use existing Java clients for GitHub etc.

[ddgenome]

Where did we land on the type for the root node? I'd still argue that the following would be more consistent with Editor/Executor path expressions.

let handler = on<MatchedEvent>("/Issue()[.state='closed']", match => {
   let plan: Plan = new Plan();
   plan.addAction("SomeAction").
      .addCommand(match.Issue().reassignTo("sylvain"))
   return plan;
})

If we don't like the implicit type for the root node here, we should rethink it in Editors.

[Lawouach]

Sure, but it still looks like the call happens at that time rather than later on so this is confusing.

Why not

.addCommand(match.reassignTo, "sylvain")

?

This states clearly that the call is not performed from within the handler.

Or have I misunderstood something?

[kipz]

@Lawouach I agree to some extent, and it is a compromise IMO.

Having said that, the signature of 'reassignTo' is:

(name: string) => Command

So it's pretty clear (when using a good TS editor) what's going on there.

We did discussed making it more like:

match.reassignToCommand("dd")

or

match.newReassignToCommand("sylvain")

But overall, the shorter version was preferred. For what it's worth, I'm with you, but don't feel strongly enough about it to miss the opportunity to close down this issue at last!

[Lawouach]

Gotcha. I think I had misunderstood this was returning Command. Makes total sense.

[kipz]

@ddgenome - this was discussed but without more input from you, we think @rod's response shows why it's the way around that it is:

#105 (comment)

i.e. there can be multiple matches, and there is always only ever one root.

Please chime in if we got the wrong end of the stick.

[ddgenome]

I think the type of the match is orthogonal to how many "things" are being returned, but they are related. If the root node is an Event, it can still only ever be one root node processed by a handler. We'd have to decide whether we would ever want that Event root node could have multiple children or to fire off individual handlers for all of its children.

This too would probably benefit from a discussion.

[ddgenome]

After talking with @kipz, I'll try to make the point I am trying to make more clear.

From @johnsonr:

Regarding the handlers, there is always a single root e.g. Issue but there can be multiple matches.

E.g. /Issue()[@State='closed']/resolvedBy::Push()/contains::Commit()

Not sure that's exactly the model but it serves as an illustration. It returns a single Issue and one or more Commit nodes. Thus the present model makes sense to me. Am I missing something?

There is always a single root, but I am asking what "type" that root should be. Currently for the path expression you show, /Issue()[@state='closed']/resolvedBy::Push()/contains::Commit(), we are returning a tree with a single root node of type Issue. This is incongruent in two ways. First, according to XPath and our path expression spec, /Issue() would be interpreted as any child of the root node with type Issue. To make the above path expression correct, it would have to be /self::Issue().... Second, when we use path expressions in Editors, Reviewers, and Executors, we have a root node much more akin to what XPath provides, whereas with Handlers it is quite different. For Editors, Reviewers, Executors, and XPath the root node is the parent of everything you specify in the path expression (Project, Project, Service, and the entire document, respectively). For Handlers, the root node differs depending on the first "child" in the path expression. So in this example, the root node is of type Issue with a closed state.

Since Handlers deal with events, I suggest we create a standard root node type for Handlers called Event. This will be the root node for all trees passed to Handlers. The rest of the tree would be refined by the path expression in the usual way. The Event type could have generic event information like a timestamp, a source event channel, etc.

The code for creating a Handler would then look something like this:

let handler = on<MatchedEvent>("/Issue()[.state='closed']", root => {
   let plan: Plan = new Plan();
   plan.addAction("SomeAction").
      .addCommand(root.child().reassignTo("sylvain"))
   return plan;
})

Where you know that the root has a single child for each firing of the handler and the child is of type Issue.

I hope this makes it more clear what I am getting at. If not, let me know.

[Lawouach]

I very much agree with your demonstration regarding the need for an Event type that would be the root node to which or tree nodes could hang from.

However, XPath specifies an expression returns the set of nodes selected, not the root node of that expression. On that front, I still hope we can keep match rather than root. I do not see why we need to return the root. If we wanted to keep straight with what XPath specifies, match would actually be a sequence where, in the example above, we'd have match[0] being the Issue node matching the expression.

Note that having a sequence if often more annoying than anything else because you want to act on a single node so we might want to simply return one item, which would be different from XPath but more friendly. Or, even better, if we could adapt the signature of the handler so that it supports match => or matches => ? Or maybe match[] =>. Not sure that's feasible though.

That's more consistent with the first half of your demonstration too I believe.

[jessitron]

XPath Assumes you have the root because how else would you do the match? We are using it for something different here: expressing all the information we need, the whole relevant relationship graph. The return from our tree expression needs to give us access to each location step in some form.

…

On Tue, Jan 10, 2017 at 2:14 AM Sylvain Hellegouarch < ***@***.***> wrote: I very much agree with your demonstration regarding the need for an Event type that would be the root node to which or tree nodes could hang from. However, XPath specifies an expression returns the set of nodes selected, not the root node of that expression. On that front, I still hope we can keep match rather than root. I do not see why we need to return the root. If we wanted to keep straight with what XPath specifies, match would actually be a sequence where, in the example above, we'd have match[0] being the Issue node matching the expression. That's more consistent with the first half of your demonstration too I believe. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#105 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABGLKT9jMhEFt4zEnvbVntoGRjaN6EnCks5rQz4BgaJpZM4LQ1w0> .

[ddgenome]

@Lawouach it is true that XPath does not return the root node, because that is what you give it. Similarly when we use path expressions in editors/reviewers/executors we already have the root node, i.e., the project or service. So perhaps we need to think about how to make the top-level event available to all handlers.

As @jessitron says, a significant difference between XPath and our tree expressions are what the runtime returns. The XPath engine returns only the leaves as an array. Our path expression engine returns the entire tree.

[Lawouach]

Alright. I think I see your points. If we do return a whole tree, then a root => makes sense.

[kipz]

Here's an example that is current working on a branch:

export let simple: Handler = {
  name: "ClosedIssueReopener",
  expression: ClosedIssues,
  description: "Reopens closed issues",
  handle(event: Event<Issue>){
    let issue = event.child()
   return new HandlerResult()
        .addMessage(new Message(issue).addExecutor(issue.reopen()));
  }
}

[kipz]

Terse version:

export let handler = on<Event<Issue>>("/issue[.state()='closed']", event => {
      let issue = event.child()
      return new HandlerResult()
        .addMessage(new Message(issue).addExecutor(issue.reopen()))
})

[ddgenome]

The terse version is only terse because it omits useful information.

[kipz]

Woot!

…

On 19 Jan 2017 18:38, "johnsonr" ***@***.***> wrote: Closed #105 <#105>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#105 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABx1QqHu1B_QqdqFIkEdRSjWT-CaoREAks5rT62qgaJpZM4LQ1w0> .