[NETBEANS-97] Don't add braces around try/synchronized blocks when reformatting

[bondolo]

Extraneous braces will also be removed according to user preference

[ghost]

Looks good to me, but someone else must approve! ;)

[lbruun]

Nice, but I'm afraid I agree with Emilian's comments on the original ticket: not everyone would want this. I cannot see from the code how you've made this into a user preference, more importantly a user preference which doesn't have other roles too. Please explain. Thx.

(again, I'm all for it, I just think it is an acquired taste which should be on opt-in basis)

[bondolo]

I agree that not everyone might want this; in part the patch is to allow people to not have extra braces inserted by the formatter where the user did not put them. The opposite side, removing braces is only enabled if the user has adjusted their "Braces Generation" preference to "Eliminate". Eliminate is an option I suspect few people use.

This patch is mostly about leaving things the way the user typed them rather than adding unnecessary/unwanted braces.

If there was to be an option, which doesn't seem necessary, it would be a "Braces Generation" checkbox "around try & synchronized".

[lbruun]

Thank you for explanation, bondolo.

You are touching something sensitive here as people rely on the formatter to do things for them. I checked out your PR and did a short test. The purpose was to see how this changes for the default IDE user using default settings:

With your PR (and with default settings) the following snippet:

public static void example() {
    if (x == y) try {
        System.out.println();
    } catch (Throwable t) {
    }
}

will now be left intact, whereas previously it would be reformatted into (braces added):

public static void example() {
    if (x == y) {
        try {
            System.out.println();
        } catch (Throwable t) {
        }
    }
}

Exactly as you claim. Check!

I also did a small regression test. The following code snippet

if (x == y) 
    System.out.println();

will be reformatted into

if (x == y) {
    System.out.println();
}

both with and without the PR. Check !

Personally I find this change in default behavior to be acceptable even if the ideal situation would be that your proposal would be 100% opt-in based. Let's see what other say. I'm imagining that your proposal would be more acceptable if you could make it completely without side-effects for existing users. Then it would be more or less impossible to argue against. But again, it is ok for me personally.

[sdedic]

I would also prefer to add an user option for the behaviour. Usage of { depends on group's code style should be somehow enformed.
But in any case, add relevant test cases to org.netbeans.modules.java.source.save.FormatingTest

[geertjanw]

If test cases are added and, ideally, an option in the Options window to enable/disable this, there'd be agreement on this one.

[ghost]

It's a neat thing, but it looks a bit unusual first. The example of Ibruun shows, that this patch removes one indentation, which means less clutter, less braces. It could be called the if-try condition.

[ghost]

Will this pull request work, if the source repository is missing due to time passed by? 💀

[cowwoc]

@bondolo @cp7781 Did anyone add the requested options dialog? I assume this is the only thing blocking a merge...?

[geertjanw]

Yes, looks like we need a way to let the user choose whether they want this behavior or not.

[bondolo]

I believe the requested tests are more important. I haven't had time to turn the presented examples from this thread into tests (or more importantly learn how to turn them in to appropriate NetBeans tests).

I am still somewhat resistant to adding an option. In the current proposed patch the effect is for the source reformatter to not insert extra braces that the user did not enter when performing reformatting, ie. the reformatter makes fewer changes to user code. If the user did include braces then they are preserved.

If user enables the existing remove braces option then reformatting will remove the "extra" braces around some try/synchronized statements, however since it also removes them from conditionals with single statement bodies it is probably not a widely used option and those who use it must really hate braces.

[bondolo]

I looked for examples of existing Reformatter tests and could not find any. I don't believe that there are any unless I am missing something.

I would also like to understand the perceived necessity of providing an option for this change. Since the net result is for the reformatter to make fewer changes to the source the user typed I am resistant. In part because I expect that non-uniform use of the option across teams will result in the undesired braces slowly creeping in to source-bases as multiple users apply reformatting.

[geertjanw]

Since, as you state, "this patch is mostly about leaving things the way the user typed them rather than adding unnecessary/unwanted braces", I see it rather as a fix than an enhancement and in that light should simply be accepted and merged.

[bondolo]

I would certainly be pleased with that outcome!

[jlahoda]

AFAIK, the tests for the reformatter are here:
java/java.source.base/test/unit/src/org/netbeans/modules/java/source/save/FormatingTest.java

[bondolo]

I have finally updated the code for this PR to add tests and also fix a side issue with the java hints to not complain about try, synchronized and switch blocks following if/else/for/while. However given the amount of time something seems to have bit rotted and I cannot update the PR with the new code.

[bondolo]

I will add some docs this weekend.

[bondolo]

Release notes documentation:

Previously the Java Source Reformatter would reformat Java source does not treat try and synchronized blocks as blocks when it encounters them with control structures. So

if (foo == bar) try {
   baz();
} catch (Exception all) {
   log.(....);
}

was reformatted as :

if (foo == bar) {
  try {
    baz();
  } catch (Exception all) {
    log.(....);
  }
}

The additional added basic block layer is not needed as the try is already a block. The redundant braces are no longer added. The same applies for a synchronized block as well:

if (foo == bar) synchronized(quux) {
   baz();
}

was previously reformatted as :

if (foo == bar) {
  synchronized(quux) {
    baz();
  }
}

but will now be left alone.

In addition to "if/else" this formatting all affects other control structures such as "for", "for-each" and "while".

Manually adding braces around a try or synchronized is still allowed and the reformatter will leave them as-typed unless the source reformatter option for Java braces has been configured to "Eliminate".