Investigate simplifying the TS Rug Parameter model

[kipz]

Currently all parameters to a TS rug are declared in an array on the Rug. While this works, and destructing can be used in simple cases, when there are many parameters, it quickly gets ugly when consuming the parameters in the function implementation (edit/review/populate etc).

At the very least, the duplication of parameter names should be removed if possible.

[kipz]

Proposal 1: Property Injection

class SimpleEditor implements ProjectEditor{
    name: string = "Simple"
    description: string = "My simple editor"
    @parameter({description: "num", pattern: "\\d+"})
    num: number = 10
    edit(project: Project){
        this.num == 10;
        project.addFile("src/from/typescript", "Anders Hjelsberg is God")
    }
}

Pros:

• Super simple. No extra types/classes required
• Can use super class for grouping

Cons:

• Magic (this is why we didn't do this originally)
  • i.e. 10 might be overwritten by value supplied by user
• Not thread safe (but hey, we can't ensure this anyway)

[kipz]

Proposal 2: Mapped types and a Parameters interface:

class MyParams implements Parameters {
  //name, type and default value from from definition
  @parameter({description: "Content", pattern: "$ContentPattern"})
  content: number = 10;
}

class SimpleEditor implements ProjectEditor<MyParams> {
    name: string = "Simple"
    description: string = "My simple editor"
    params: MyParams = new MyParams()
    edit(project: Project, p: RugParams<MyParams>)  {
        p.content == 2;
        project.addFile("src/from/typescript", "Anders Hjelsberg is God")
    }
}

Pros:

• Could maybe reuse instances of editors for multiple requests/threads
• Grouping via inheritance could be easy

Cons:

• Has a separate class, more code etc.

[kipz]

Proposal 3: Status Quo:

https://github.com/atomist-rugs/travis-editors/blob/master/.atomist/editors/EnableTravisForRugArchiveTS.ts

Pros:

• It's the status quo
• Makes composition of parameters super easy

Cons:

• Duplication of parameter names
• Manual type coercion in interface
• Need synthetic interface or verbose destructuring

[kipz]

Or any others?

[cdupuis]

@kipz, I like option 1 but worry about thread-safety.

Right now ProjectOperations are being cached in our runtimes. So I guess on execution we would need to create new instances of the JS Object, or so?

[jessitron]

@cdupuis isn't caching instances dangerous anyway? They could mutate fields on the editor object at any time.

[jessitron]

I like option 1 except for inheritance as a grouping mechanism. but it's fine.

[cdupuis]

The API in Rug and ProjectOperationArchiveReader was intended to allow that. But this came out of the DSL-only days. Maybe that assumption isn't valid any longer but re-creating the entire operations thing every time is not an option as this takes too long.

I think if we can't guarantee thread-safety, Rug needs to handle that internal by creating new instances on invocation.

[kipz]

@cd - I think we can do that, and as @jessitron - we should already be doing that anyway.

So rather than exporting an instance of the class, we'll just export the class itself, and create a new one for each call.

[cdupuis]

sounds good. yeah, that is indeed safer.

[ddgenome]

I'd be happy with 1 or 3.

[kipz]

Proposal 1: property injection wins
#229 (comment)

Though we should be doing this already, we'll need to ensure that:

• We create and reuse the ScriptEngine instance
• Use separate bindings for each thread
• Create a new instance of each rug for each operation (class to be export instead of instance)