https://github.com/groovy/groovy-eclipse/issues/603

[ybayk]

A new boolean property isBuilder has been added to the method contribution.
When it is not specified or set to false, the existing behavior is completely preserved.
isBuilder is similar to noParens. It removes parentheses around arguments, however parentheses are forced when the method has a trailing closure.

When isBuilder=true is specified, the following behavior is expected:

1. For single-parameter methods with the parameter having Closure type
   e.g. method name: 'bar', type: 'void', isBuilder: true, params: ['block':Closure]

• Selecting the proposal will lead to the state where the user can immediately start coding the closure block which is desired in a "builder" use case.
  bar { | }
• No parentheses around the closure parameter regardless of the Content Assist preferences.
• it is suppressed from the proposed closure literal

2. For multi-parameter methods with a trailing Closure parameter:
   e.g. method name: 'bar', type: 'void', isBuilder: true, namedParams:['name':String], params: ['block':Closure]

• Selecting the proposal will lead to the parameter editing mode:
  
• After completion (Enter key) the caret will be automatically placed in a middle of the Closure block.
• Parentheses are forced around the parameter list, but it excludes the trailing closure regardless of the Content Assist preferences.
• it is suppressed from the proposed closure literal

3. For methods without a trailing Closure, behavior is identical to noParens=true
   e.g. method name: 'bar', type: 'void', isBuilder: true, namedParams:['name':String], params: ['regular':String]
   The resulting proposal will look like the following:
   bar name: name, regular

• No parentheses around the parameters

The above change will make groovy builders generally expressed in DSLD. Also it will make builders less time consuming and less confusing to use by DSLD consumers.

[eric-milles]

So what's the difference between noParens and isBuilder? Would you ever want both? Is it invalid to set noParens if isBuilder is set (to true or false)?

[eric-milles]

Should it be builder instead of isBuilder? noParens and useNamedArgs are both Boolean and don't use "is" prefix.

Do you see a problem with 3 flags to control all this stuff? We have 8 possibilities at the moment and it would go to 16 if another option was added. Are the 3 options independently variable? If not, I would think some sort of Strategy pattern should be used instead and deprecate noParens and useNamedArgs.

[eric-milles]

It seems like useNamedArgs was added to support builders. It is used by the SwingBuilder DSLD. I'm not sure if it predates the namedParams map, but it does predate noParens which was added to support Groovy 2 command chaining.

[ybayk]

• isBuilder vs noParens - shortly mentioned above, they are similar but there are a few differences:

1. when more than one params with a trailing closure e.g. foo(String bar, Closure block)
   noParens uses comma separating the closure: foo bar, { it } and this is the main reason why I had to introduce a new flag
   isBuilder forces parentheses around all params but the closure: foo(bar) { }
2. isBuilder suppresses it
3. isBuilder exclude the trailing closure from the "parameter editing mode" - it is really treated as a code body - not as a parameter
4. Upon completion of parameter editing, caret is moved in a middle of the closure brackets - not after them (as in noParens)

• due to difference of handling parentheses, isBuilder overrides behavior of noParens, so noParens=true does not have any effect when isBuilder=true.
  I guess it is possible to deprecate noParens and introduce a strategy field (e.g. commandChaining | isBuilder), but to do it clean (using a strategy class) GroovyJavaMethodCompletionProposal/ParameterGuesserDelegate should be heavily refactored which I am not sure whether it is worth it. We might also consider introducing an enum here along with noParens deprecation, but I doubt we will ever have more than two items. Let me know what you think.

• regarding exploding of the flags - I do not consider it as evil in this case. More flags give more freedom of expressing DSLDs. Relative complexity and variety of options is compensated by ease of use by DSLD consumers.

• isBuilder vs builder - yes we can rename to builder following existing convention, my original thought was builder as an argument usually implies a builder object, and isBuilder would clearly indicate a flag.

• will work on follow-up change for groovyXX/plugin_dsld_support and will include missing new attributes such as useNamedArgs

• useNamedArgs - this option exposes a corresponding preference setting and seems to enable printing parameter labels followed by colon in the parameter list. It is independent from any other flags

• there is actually one more preference flag left unexposed: guessArgs. Like useNamedArgs exposing a corresponding preference flag, it could be used to guess parameter values regardless of the Code Assist preference settings.

[eric-milles]

I'm not against freedom of choises/expression. What I am concerned with is too many combinations that are not really valid or useful. I'd prefer to see noParens and useNamedArgs deprecated and replaced by an enum or class (aka strategy) if there are really only 3 states we wish to support. You mention that setting isBuilder essentially means that noParens is ignored.

Also, your edits include changes to delegatesTo but none of your tests exercise it. Did you intend to modify delegatesTo or was it done because MethodContributionElement constructor signature changed?

[eric-milles]

How far off is useNamedArgs from the strategy you are proposing for isBuilder? I wonder if one or two changes to it could give you what you want without the introduction of 2x the combinations to support/test.

[eric-milles]

Here is my frustration: We did not really get a chance to discuss solution options for #603. Now I am having to review changes in hand, when I don't think we have completely understood the problem.

[ybayk]

As I mentioned useNamedArgs is used to force argument names for parameters. Does not seem to have anything related to the builder pattern. Here is the relevant snippet:

groovy-eclipse/ide/org.codehaus.groovy.eclipse.codeassist.completion/src/org/codehaus/groovy/eclipse/codeassist/completions/GroovyJavaMethodCompletionProposal.java

Lines 285 to 294 in c9406f3

	if ((fPreferences.isEnabled(GroovyContentAssist.NAMED_ARGUMENTS) || i < namedCount) && i != indexOfLastClosure) {
	buffer.append(nextName);
	if (fPreferences.isEnabled(DefaultCodeFormatterConstants.FORMATTER_INSERT_SPACE_BEFORE_COLON_IN_LABELED_STATEMENT)) {
	buffer.append(SPACE);
	}
	buffer.append(":");
	if (fPreferences.isEnabled(DefaultCodeFormatterConstants.FORMATTER_INSERT_SPACE_AFTER_COLON_IN_LABELED_STATEMENT)) {
	buffer.append(SPACE);
	}
	}

Example:
method name: 'bar', type: 'void', useNamedArgs: true, isBuilder: true, namedParams:['name':String], params: ['baz':String]

Proposal generated:
bar(name:name, baz:baz)

Note that baz is actually a regular parameter forced to be named! I am not sure really what is the use case for that, but for sure it is not relevant to builders and can be applied in combination

[ybayk]

Added missing tests for delegatesTo and tests against useNamedArgs with a builder on/off.

[eric-milles]

Your first two examples in the initial comment can be attained with useNamedArgs. Given method name: 'bar', type: void, useNamedArgs: true, params: ['name':String, 'block':Closure] content assist would insert: bar(name: null) { }

When method does not have trailing closure, isn't it for the user to decide if parenthesis are desired (via content assist pref)? Otherwise, noParens exists to remove.

I'm not sure I understand wanting parens in one case and not in another. What I am driving at is there are at least 2 settings in the DSLD and several more in content assist prefs to control this stuff. But you seem stuck on associating a tuple of these settings to 1 single flag. But if I get a request for another mix of settings how would I handle that?

As I stated in #603, I'd prefer to tackle each of the content assist problems as separately as possible and I would choose new DSLD API as a last resort solution. It may be beneficial for named arguments or trailing closures in general to make a non-DSLD change.

[ybayk]

Regarding parens vs no parens - that is a pretty conventional Groovy builder approach:
Parentheses are simply required by Groovy syntax if you do not want to use comma as a trailing closure separator - here is an example for a Gradle build:

compile("org.codehaus.groovy:groovy-all:2.5.0"){
    force = true
}

Alternative which is provided by noParens is practically never used in builders:

compile "org.codehaus.groovy:groovy-all:2.5.0", {
    force = true
}

[ybayk]

Your example with useNamedArgs

1. Converts regular parameter into a named one which is not desired at least for builders - regular parameter should stay regular
2. Assumes parameter guessing is turned on in Code Assist preferences (which may or may not be desired by the user)

[ybayk]

Note that besides formatting, isBuilder flag also triggers a trailing closure treatment as a code body. The closure is excluded from the argument editing mode, and the caret moves right inside the block as soon as editing is completed.

[ybayk]

Good read about builders is here:
https://books.google.com/books?id=RNtOCwAAQBAJ&pg=PA255&dq=groovy+for+domain-specific+languages+buildersupport&hl=en&sa=X&ved=0ahUKEwjP9rnlz4vcAhWTPH0KHQvrAgMQ6AEILTAB#v=onepage&q=groovy%20for%20domain-specific%20languages%20buildersupport&f=false

[eric-milles]

I understand how each property works. I don't really want to read the book on builders. What is your use case that is not being met? You keep referring to builder syntax in the general sense. Can you break that down to specific items, one issue per item that is not achievable with current DSLD and content assist preferences?

Personal preferences for completion and formatting should remain controllable by the user. I have a hard time with wanting to set content assist preferences a certain way and then override them under specific circumstances. If the user wants parameter guessing, why should it be disabled?

Are you not able to join the Slack channel so we can have immediate back and forth so I can ask for examples and then ask about the examples that are shown?

[eric-milles]

Please understand that I did not design the DSLD stuff, so I am having to live with some questionable choices like useNamedArgs, noParens, and so forth. But since they exist, any new development should keep them in mind.

[ybayk]

Regarding minimizing DSL API changes, doable but we would have to change behavior of noParens:

1. foo bar, { } => foo(bar) { }
2. treat trailing closure parameter as code block (exclude from parameter editing, position the caret)

Are you ok with that?

[eric-milles]

Is your top concern the trailing closure handling? If so, can you open a new issue just for that and describe current behavior and desired end state?

[eric-milles]

In general, are you desiring to add method contributions with noParens: true and the conflict is trailing closures? Do you have a mini DLSD that adds a method with trailing closure and another wihtout?

[eric-milles]

Right now, I am unable to explore the problem space and experiment with possible solutions. I would like to see the problem space divided up, instead of just dealing with "builder syntax" in its entirety.

[ybayk]

Yes, exactly I have only these 2 concerns:

1. Conflict with noParens - comma vs parens.

2. Handling a trailing closure as code block (only when it is outside of the parens):
   That is when foo(bar, { }) - treat it as an argument
   When foo(bar) { } or foo { } treat the closure as a code block.

[eric-milles]

So for 1, what is the conflict? In general do you want to add methods that omit parens, but with trailing closures, you want them? Can't the trailing closure methods be specified without a noParens setting?

[ybayk]

With noParens used, parens surround the trailing closure:
method name: 'foo', type: 'void', params: ['bar':String, 'block':Closure] => foo(bar, block)

This is the default eclipse behavior. You can get it very close if you enable PreferenceConstants.CODEASSIST_GUESS_METHOD_ARGUMENTS and GroovyContentAssist.CLOSURE_NOPARENS in Eclipse preferences:

It will turn to foo("") { }

Or if GroovyContentAssist.CLOSURE_BRACKETS is turned on instead of PreferenceConstants.CODEASSIST_GUESS_METHOD_ARGUMENTS

It will turn to foo(bar) { it }

Here is the problem with code assist preferences:

1. they are global, affects all of you autocompletions
2. what we need is ability to generate specific builder templates, so I believe DSLD is a the most appropriate place to model them
3. If we are unable to model them in DSLD we have to also supply instructions to turn on such and such preferences in UI, and then the user would have to deal with problem 1

[eric-milles]

Is foo(bar: something) { } not workable? Given method name: 'foo', params: [bar:String, block:Closure] what if we just triggered on trailing closure having name "block" or "code" (a convention instead of an explicit setting)?

It sounds like the desire is to override content assist preferences to prevent requiring user make specific setting adjustments.

If you want templates, there is a templating system. They would not be context sensitive -- meaning you could not specify in which context a particular template applies. They would all be proposed at all levels as long as the completion expression prefix matches.

[ybayk]

Yes foo(bar: something) { } is a desired output, when parameter list has more than a closure.
I am fine with the convention triggered by code or block argument names (or either). Would you consider caret handling to be triggered along?

[eric-milles]

If you want foo(bar: something) { } you can use method name: 'foo', params: [bar:String, block:Closure], useNamedArgs: true.

If you want foo(bar) { } for method name: 'foo', params: [bar:String, block:Closure] then we are very close, I think. Default content assist preferences plus checking "Place trailing closure arguments after closing parenthesis" yields: foo(bar) block

The block could be replaced with a closure literal either because this is a DSLD contrib or because the name convention. I have thought that a name inserted outside the parens is pretty useless. Maybe we could just fix that in all cases. Thoughts?

[eric-milles]

For this proposal, I am turning a blind eye towards the guessing preferences.

Also, as you mentioned in your summary, checking "Use closure literals for closure arguments" yields: foo(bar) { it } with selection covering "{ it }". This case could probably be fixed with little effort if the one above was completed.

[ybayk]

I definitely do not want foo(bar: something) { } when bar is a regular parameter, only when it is a named one.
Also would like to avoid forcing user changing preferences (in your example it requires Groovy's "Use closure literals for closure arguments" and Java's "Insert best guessed arguments" to be both ON.

[ybayk]

Sorry, i think we hit "Comment" at the same time :) Ok, so what about just naming convention that would trigger:

1. foo(bar) { } without having to change preference?
2. handle the caret after argument list editing is completed

[eric-milles]

Okay, I am sensing you want immunity from user's content assist preferences. But can we ignore the guessing stuff for now? And is it so bad to require the user to check "Place trailing closure arguments after closing parenthesis"? It would still be functional syntax to have the block inside (though not idiomatic or whatever).

I can see ignoring the setting for "Use closure literals" and just provide the literal since it is required for correct syntax when outside the parens. So, if we fixed that up, would there even be a need for a name convention?

[eric-milles]

Assuming "Place trailing closure arguments after closing parenthesis" is checked, I see 2 small issues to write up and fix first:

1. "Use closure literals for closure arguments" unchecked gives foo(bar) block -- replace "block" with literal "{ }" and selection/caret goes inside literal instead of covering it
2. "Use closure literals for closure arguments" checked gives foo(bar) { it } -- replace "{ it }" with "{ }" and selection/caret goes inside literal instead of covering it

Fair enough?

[ybayk]

Not ideal. DSLD would have to come with extra instructions on how to make them work really.
How about just exposing "Place trailing closure arguments after closing parenthesis" (e.g. noParensAroundTrailingClosure) in DSLD and fixing the literal when it is turned on from either Preferences or DSLD?

[eric-milles]

The 2 non-default guessing modes can either continue to work as they do or ignore user preferences... I'd prefer the first option.

[ybayk]

Or just fixing non-idiomatic noParens trailing closure use case?

[eric-milles]

But see, if "Place trailing closure arguments after closing parenthesis" is in its default state, you get working code. Either foo(bar, block) or foo(null, { }).

I don't see noParens coming into play here. If you tell the DSLD to excluse parens right now, it does so. I consider that flag working properly.

[eric-milles]

Like I sad above, first steps are to assume a base state. We can cover more permutations of content assust prefs later.

[ybayk]

it does work but that kind of code practice foo bar, { } is mostly unused.

[eric-milles]

If I set noParens to true and got parens I'd be scratching my head.

[ybayk]

That would be only limited to trailing closure use cases, which will be apparent to not have parens.

[eric-milles]

We could set "Place trailing closure arguments after closing parenthesis" as default checked too.

[eric-milles]

Do my 2 proposed issues seem like a reasonable first step? I think they would be easy to code up, test and review. Then we could regroup on remaining gaps/issues.

[ybayk]

Defaults would be fine with me, hopefully it is not going to be too radical...

[ybayk]

that would leave us with it suppression and caret handling...

[eric-milles]

Would you like to write these up as new issues or would you like me to do it? We'll put non-default guessing aside for now and leave noParens as-is.

Assuming "Place trailing closure arguments after closing parenthesis" is checked, I see 2 small issues to write up and fix first:

1. "Use closure literals for closure arguments" unchecked gives foo(bar) block -- replace "block" with literal "{ }" and selection/caret goes inside literal instead of covering it
2. "Use closure literals for closure arguments" checked gives foo(bar) { it } -- replace "{ it }" with "{ }" and selection/caret goes inside literal instead of covering it

[eric-milles]

At this point I am not seeing a DSLD API change; the fixes would apply for method completion when the preferences are in the state described.

[eric-milles]

DSLD method contributions appear to be immune to "Use named arguments for method calls" pref, so that's one less pref to worry about.

[eric-milles]

One additional note, if "Place trailing closure arguments after closing parenthesis" is checked and "Use closure literals for closure arguments" is unchecked, typing { instead of enter to select the completion proposal yields foo(bar) { it }, with "it" as the last tab group (not "{ it }"). So there is handling in there somewhere for this. The { trigger behavior seems like what you're after. Can you try it out?

[ybayk]

I can check handling of '{' as a trigger, but it is not desired when you are building a tree of nested calls. it has to be explicitly deleted every time you start a new node. So behavior of parameter guessing where only { } is inserted is preferred in this case. Also it is always proposed if you hit ctrl-space inside a closure.
But at the same time I understand that for non-DSLD use cases like .collect{ it }, automatic it could still be handy. This is one of the reasons why a preference does not seem to be sufficient.

[ybayk]

Anyways I will refactor PR, to fix just issues you outlined and the we can resume brainstorming from there.

[eric-milles]

I'm not saying the { triggering is perfect, but it does give a hint that what you are after is just a step or two away.

I'm asking for 2 issues to be created and new PRs coded, tested and reviewed for them. This PR can be left alone, in case you want to look at your changes again.

[ybayk]

to clarify, you want one issue for checked state of the “use closure literal” option and another for unchecked to both end up with { }, and 2 PRs, that granular?