-
Notifications
You must be signed in to change notification settings - Fork 13
Harmonise Editor, Generator, Executor and Handler TypeScript APIs #105
Comments
Example of current API: Issues:
Benefits:
|
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..
}
} |
it returns the message <3 <3 |
It still has atomist as a magic global var? If it's going to return data, it could take atomist as a parameter too? |
Oh yeah, updated. |
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..
}
} |
|
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)
}
} |
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. |
So would the handle function return an |
The only collections are arrays, but we could support returning a single message or an array of them.
yep.
I think the only way to support an inline expression is to have a concrete var to operator on, which implies a magic var.... |
With more concrete match: let handler: Handler = {
query: "/issue[.state()='closed']",
handle (match: MatchedIssue){
return new MessageBuilder(match.root())
.withAction("AddLicense")
.withAction("HelloWorld");
}
} |
pretty! |
@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) |
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
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")
} |
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.
Yep. So TS supports union types, so the type could include any other Actionables you can imagine.
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. |
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 |
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?) |
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. |
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 |
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 If we're going to get rid of the use of A replacement would be to expose a I think we need to make a call on this before moving too much further forward with the design. |
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: The tree expression for a handler seems to have the root node explicitly: 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 |
I don't know. I see what you mean @ddgenome but at the same time the interface does say 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:
where the we still act on the Issue node which isn't the root. |
From a meeting with @cd and @kipz and @slimslenderslacks today: All commands will (eventually) be asynchronous. Outstanding question: We also talked about whether we want to add methods to treeNodes or supply TypeScript functions that accept treeNodes. (Either can return/record commands) |
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. |
Sure, but it still looks like the call happens at that time rather than later on so this is confusing. Why not
? This states clearly that the call is not performed from within the handler. Or have I misunderstood something? |
@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! |
Gotcha. I think I had misunderstood this was returning Command. Makes total sense. |
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. |
After talking with @kipz, I'll try to make the point I am trying to make more clear. From @johnsonr:
There is always a single root, but I am asking what "type" that root should be. Currently for the path expression you show, 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. |
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 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 That's more consistent with the first half of your demonstration too I believe. |
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>
.
|
@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. |
Alright. I think I see your points. If we do return a whole tree, then a |
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()));
}
} |
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()))
}) |
The terse version is only terse because it omits useful information. |
Handler JS/TS programming model as per #105
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>
.
|
No description provided.
The text was updated successfully, but these errors were encountered: