Want a way to disable "combining closures with only one statement into one line"

[basil]

I use the Eclipse Groovy formatter via diffplug/spotless to automatically format my Gradle scripts (and other Groovy DSL scripts, such as Jenkins jobs) through Gradle. Unfortunately, there is an issue in the Eclipse Groovy formatter that prevents it from being suitable for formatting most Groovy DSL scripts, such as Gradle scripts, Jenkins Job DSL job definitions, and Jenkins pipeline definitions. This is a big market for the Eclipse Groovy formatter, as I am not aware of another solution for formatting Groovy source files that can be run easily from the command-line.

To see what I mean, take a look at this chapter in the Gradle documentation. Consider the examples provided there, which are typical examples of Groovy DSL scripts.

Example 1:

repositories {
    jcenter()
}

Example 2:

repositories {
    maven {
        url "http://repo.mycompany.com/maven2"
    }
}

Unfortunately, the Eclipse Groovy formatter will "combine closure[s] with only one statments [sic] with less than 5 tokens to one line". This results in the above snippets being formatted as follows:

Example 1:

repositories { jcenter() }

Example 2:

repositories { maven { url "http://repo.mycompany.com/maven2" } }

This makes the code more difficult to read. Similar examples could be given for Jenkins jobs.

I tried to find a way to disable this behavior, but (unfortunately) I found there was no preference for it. GroovyBeautifier#combineClosures always goes down the code path linked above, and GroovyBeautifier#getBeautifiEdits always calls GroovyBeautifier#combineClosures. Thus, in the current codebase, the only way of disabling this behavior is to disable the use of GroovyBeautifier entirely and use the Eclipse Groovy formatter just to perform indentation, not formatting. But this also disables many other useful features, such as formatting lists, correcting braces, and removing unnecessary semicolons. Therefore, I propose we add a way to opt out of the "combine closures with only one statement to one line" functionality without opting out of the many other benefits of GroovyBeautifier.

I considered making the hard-coded constant 10 in the relevant code customizable. The comment says that this "combine[s] closure[s] with only one statments [sic] with less than 5 tokens to one line", so presumably the number 10 in the code is related to the number 5 in the comment. Whether the comment is wrong or I am misunderstanding the code isn't clear, but one thing that is clear is that the token count is what is under consideration here. I don't think that most users who are customizing their formatter are concerned with internal implementation details like token count. In fact, exposing the token count to the user seems like it might be a bad idea if the implementation is ever changed to do something more clever (like taking into account line length), since then the option would have to be deprecated and removed. Instead, it made more sense to me to expose a boolean called "combine closures with only one statement to one line". This hides the implementation details of token count from the user and enables the implementation to be refactored later to use a better metric (for example, one that takes into account line length), while still allowing users to opt out of the undesired functionality today.

[eric-milles]

Do you see much value in the "combine closures" functionality? What if it was removed instead of making a new preference? I'm not much of a fan of the Formatter preferences page and would really prefer not to have any controls added to it without some serious design consideration.

[basil]

I'm not much of a fan of the Formatter preferences page

Thanks for the quick reply. But before I address your questions, could you please clarify the reason why you’re not much of a fan of the Formatter preferences page?

[mauromol]

I personally use syntaxes like repositories { jcenter() } very often. Think of something like collection.findAll { }.collect { ... }, when the closure bodies are just one-line. If Greclipse formatter forced them to be multi line without any opt-in/opt-out ability, it would be quite annoying.

[eric-milles]

@basil Re formatter preferences page. Groovy's page is just a hastily thrown together set of checks or radios. Java's formatter config dialogs is practically an application of its own. I'd rather see Groovy inherit Java's settings and add only special Groovy-specific settings, much like is done for Content Assist and Syntax Coloring.

Here is Groovy's:

And here is Java's:

[basil]

I see what you mean about the existing Groovy preferences page not looking as good as the Java one. This is a pre-existing issue, and to fix it one should redesign the Groovy preferences page. Whether or not I add an additional checkbox below the existing “remove unnecessary semicolons” checkbox shouldn't meaningfully impact this effort.

Regarding your earlier question, I don't see much value in the "combine closures" functionality for Groovy DSL scripts; however, I do see value in it for regular Groovy code. As @mauromol pointed out, it is common to use syntax like collection.findAll { }.collect { ... } in regular Groovy code. If the “combine closures” functionality were removed instead of making a new preference, this would be a regression for that use case.

[eric-milles]

@basil what I am understanding from this issue is you don't want the "force compact" behavior of the formatter and @mauromol is saying he would not want a switch to "force expand". To me, this looks like a case where the formatter should leave the code as the user has chosen to lay it out. If a particular closure literal is compacted, leave it that way; maybe just correct indentation. If another closure in the same file is written across multiple lines, also leave it that way.

So, I still think that a preference is not the way to go. Does this make sense? I'm not concerned about taking away the "force compact" behavior; I don't see it as a regression if the updated formatter does not force in the other direction. But instead preserves closure literals line wrapping.

[mauromol]

So, I still think that a preference is not the way to go. Does this make sense? I'm not concerned about taking away the "force compact" behavior; I don't see it as a regression if the updated formatter does not force in the other direction. But instead preserves closure literals line wrapping.

I understand your point of view, however what about the maximum line width?
Regarding actual source code, the JDT formatter usually makes you decide to "always wrap", "wrap if necessary", or "do not wrap". It may even let you decide to force split/wrapping, even if the maximum line width has not been reached.

With some other cases, like Javadoc, it may however be less complete. Consider the following example: suppose you have a one-line Javadoc comment:

/** Hello there */
public class MyClass {
}

If that Javadoc comment is encountered by the formatter, it is left as is, unless it's longer than the maximum line length. In that case it's wrapped. But if you write:

/** 
  * Hello there 
  */
public class MyClass {
}

the formatter will never compact that comment as a one-liner.

However, when you consider actual code, the formatter may be used to enforce a unified style and convention among team members, so letting it reorganize your code following some well defined constraints may indeed be useful and desirable. As an example, for method declarations you can decide, for instance, to make the formatter reorganize method parameters so that they are always compacted, or rather wrapped when needed or even to force split even when shorter than the maximum line width.

In our case, we're talking about code. And yes, we may simply decide to "do nothing", respecting the one-line style or multi-line style, but, as said, this makes the formatter less useful.

A complete formatter may probably allow the user to choose among:

• do not wrap one-line closures (i.e.: always use compact form, even if you break max line length)
• wrap only if needed (i.e.: when you go beyond the maximum line length)
• always wrap (i.e.: always use the expanded form)

This said, I perfectly know that the Groovy formatter would need a lot of work and that resources are not available for it...

[basil]

I agree with @mauromol that depending on the use case, one might want to do one of the following:

1. Do not wrap closures (i.e., always use the compact form, even if this results in exceeding the maximum line length).
2. Wrap closures only if needed (i.e., use the compact form if this does not exceed the maximum line length, and use the expanded form otherwise).
3. Always wrap closures (i.e., always use the expanded form).
4. Do nothing (i.e., preserve the user's wrapping choice without modification).

As @mauromol also pointed out, supporting all of these use cases would need a lot of work. The existing code base supports (1) but does not support (2), (3), or (4). PR #722 maintains support for (1) while also adding support for (3), and it shouldn’t meaningfully impact any future efforts to add support for (2) and/or (4). As such, it is strictly better than the existing code.

[eric-milles]

In the long term, yes the Java-style selections are desired. In the short term, I don't want to see any new preferences added. You could write your change in a way that leaves support for "do not wrap" in place but not selectable from UI. That is what I am asking for -- formatter changes but not preference or UI changes.

[basil]

In the short term, I don't want to see any new preferences added.

Thanks for the quick reply. But before I address your feedbacl, could you please clarify the reason why don't want to see any new preferences added?

[eric-milles]

See my first comment. In general the formatter is not in a good state; any changes should simplify. To that end, I don't want any options added. Current behaviors can be corrected to be more useful. That is why I think the compacting of closures under a certain number of tokens should be ditched in favor of leaving closures formatted as they are. The choice is left up to the user to either compact them to a single line or write them across multiple lines. And the formatter will not interfere with their choices.

[basil]

Thanks for your reply. I went back and read your earlier comment again. To your earlier comment, I had previously responded: "This is a pre-existing issue, and to fix it one should redesign the Groovy preferences page. Whether or not I add an additional checkbox below the existing 'remove unnecessary semicolons' checkbox shouldn't meaningfully impact this effort." Do you agree or disagree with this statement? And if you disagree, why?