Inconsistencies in DSL syntax

[mrueegg]

User reported:

In our beta testing ticket you asked me for a feedback about the new requirements syntax. I think this is a bigger issue deserving a separate ticket. I find the DSL syntax to be a bit inconsistent, confusing at times and not always following the online documentation. I've spent last couple of days working with it so I already know my way around it but it required me a lot of trial and error and digging through DSL source code. When we fully adopt the plugin I'd like to delegate ownership of the DSL files to our developers so it's important for me that the code is consistent and well documented.

Let me give you a couple of examples.

Some elements are defined as a method without named parameters and without a closure

variable('some_variable_key', 'some_variable_value')

Some elements are defined as a method with named parameters and without a closure

custom('some.plugin.key:task') {
    configure(
        fieldKey: 'field value',
        anotherKey: 'another value'
    ) ...

Some elements are defined as a method with parameters and with a closure

bitbucketServer("foobar") {
    projectKey 'sb'
    repoSlug 'foobar' ...

Usually this parameter can be explicitly named:

bitbucketServer(name: "foobar") { ...

or be just a string which will be implicitly converted to 'name' field:

bitbucketServer("foobar") { ...

However, sometimes it is not clear (without looking at DSL code) which parameters are converted to which fields. For example, this works:

job('SIMPLEJOB') {
    name 'simple job' ...

This works:

job('SIMPLEJOB', 'simple job') { ...

This doesn't work:

job(key: 'SIMPLEJOB') {
    name 'simple job' ...

This works:

job(key: 'SIMPLEJOB', name: 'simple job') { ...

So sometimes those implicit parameters are names (stages, artifact definitions), sometimes - keys (jobs, plans), sometimes - descriptions (scripts, triggers).

I the latest SNAPSHOT, the requirements are defined in yet another way - one of the named parameters needs to be an instance of a new object:

requirement(capabilityKey: 'hostname', matchType: new Requirement.Equals("myhostname")) {} 

Now, I'm not sure exactly how to solve this problem so it's probably worth it to put this in Github ticket so other users could contribute to the discussion.

My initial idea would be to completely remove parameters from methods and only use closures. Something like:

plan {
    key 'TEST'
    name 'Test Project'

    variables {
        variable { 
            key 'some_key'
            value 'some value'
        }
        variable { 
            key 'some_other_key'
            value 'some other value'
        }
    }

    scm {
        bitbucketServer {
            name 'Foobar repo'
            projectKey 'sb'
            repoSlug 'foobar'
        }
    }

    stage {
        name 'Stage 1'

        job {
            key 'JOB1'
            name 'Job 1'

            requirements {
                requirement {
                    capabilityKey 'hostname'
                    matchType Requirement.Equals    // enum, not constructor
                    matchValue 'myhostname'
                }
            }

            tasks {
                custom {
                    taskKey 'some.plugin.key:task'
                    configure {
                        fieldKey 'field value',
                        anotherKey 'another value'
                    }
                }
            }
        }
    }
}

This will result in more verbose code but it will be more consistent, easier to write and document. I'm curious what you think about it.

[mrueegg]

I think you have some valid points here. Let me explain the current syntax and give you some background about the decisions that led to the current design. At the beginning we didn't use named arguments, resulting in the syntax like follows:

job('SIMPLEJOB', 'simple job') { ...

I wasn't happy with this syntax because it is absolutely non-obvious without looking at the documentation (especially when you are not working within an IDE where you can just hover over the method call to see JavaDoc) what these parameters mean. This is why I provided the named argument syntax and deprecated the syntax above (I thought that I will remove the former syntax with release 2.0.0):

job(key: 'SIMPLEJOB', name: 'simple job') { ...

Another thing I wanted to achieve with the named argument list and the closure is to put all mandatory arguments for a Bamboo element (like a job) in the named arguments of the method, while the optional parameters should be part of the closure. This is probably not obvious, and I'm not sure if this decision is worth the inconsistencies. What do you think?

I'm also not very happy with the job requirement capability syntax with regards to the "new object" syntax. I normally use enums for stuff like this when multiple values are possible for a parameter. But in this case the parameters need to be configurable (e.g., with the equals match value or regex), so I chose this syntax. I'm not a Groovy expert, maybe there is another way to achieve this with enums. But maybe it is better to go with a closure anyway:

requirement {
  capabilityKey 'hostname'
  matchType Equals
  matchValue 'myhostname'
} 

What do you think? We would loose the mandatory options feature, but it would lead to a more consistent DSL I think... I would really like to get the syntax to a point with version 2.0 of the plug-in where we don't need fundamental DSL changes anymore (probably this is wishful thinking in API design world :-)).

[cattz]

I know this is not helping a lot, but TBH, I prefer the old syntax:

script('echo hello world') {
  // ...
}

versus the new one:

script() {
  description 'echo hello world'
  // ...
}

When folding this in your IDE, the old style does not need any comments for you to know what it's doing:

script('echo hello world') {...}
script() {...}

[mrueegg]

I see, this is a good argument. The intention was to have all mandatory arguments in the function call while having the optional ones in the closure - and to have this consistent everywhere. I have to rethink this...

What do you think about that idea of a customer I mentioned above to have every option in the closure? Unfortunately, you would lose the folding possibility with that as well. Beside that, how do you like that idea?

I would love to see this part of the DSL getting stable for version 2.0 of the plug-in.

[cattz]

I'm just learning Groovy, so maybe this is not possible: would it be possible to have the arguments either as named parameters or in the closure? When do you expect to get 2.0 out? or the next release, if it's not 2.0

[mrueegg]

@cattz This would indeed be possible by providing method overloads for both scenarios:

// for named arguments
def plan(Map<String, String> params, Closure closure) {
}
// as options (i.e., methods) inside the closure
def plan(Closure closure) {
}

But I would probably need to return different objects because it would be confusing for users if they have configured the plan key and name and in the returned object they could still call the name and key methods they have already specified.

I will think about this for the upcoming 2.0 release. We don't know yet when we will release 2.0. Probably we will have another version in between to test the API changes and to deprecate the old syntax.

What do you think?