Feat: Use JBang-fmt to format sources

[cstamas]

This can be easily incorporated into git commit hooks as well.

See https://github.com/jbangdev/jbang-fmt

This is "bare minimum" setup, to format only main and test Java sources of server (excluding jOOQ generated ones).

Once we format, initial formatting will need a "source format" commit without any other code change, and add that commit hash to .git-blame-ignore-revs file in repo root to fix blame.

Git integration doco:
https://github.com/jbangdev/jbang-fmt#git-integration

[netomi]

I did update the used format to a custom eclipse format with some modifications that I would prefer.
Please take a look, there is a script in server/scripts/format.sh to format the sources.

I am not happy with the array initializers. The closing bracket is not on a new line when wrapping, but that does not seem to be supported by the eclipse format. Either always have the closing bracket or not, there is no option to conditionally add it to a new line depending on whether it was wrapped.

There are a few occurrences in the code I do not really like but it looks like 99% of the other cases are fine.

wdyt @cstamas @gnugomez @autumnfound ?

you can just run server/scripts/format.sh and do a git diff

[netomi]

this format is as close as possible to the existing formatting that is currently used afaict.

[cstamas]

I like it! And make it applied as soon as possible!

[netomi]

I also added an sort import step using spotless which is run via jbang as the eclipse formatter does not support that unfortunately.

as there is no cli, claude came up with a small java file to run the ImportOrder step on all the source files that can be executed with jbang.

pretty neat imho. I pefer the following order:

// Import order: java/javax first, then everything else, then org.eclipse.openvsx,
// then static imports, each group separated by a blank line.
"java|javax", "", "org.eclipse.openvsx", "\\#"

[netomi]

btw. created eclipse-jdt/eclipse.jdt.core#5195 for the issue in the eclipse formatter that it can not place the closing brace of an array initializer on a new line if its wraps.

[netomi]

I pushed some more change to add also the spotless plugin to support 2 ways to format the source in an identical way:

• using scripts/format.sh for quick formatting in a git hook without running a gradle build
• using spotless gradle plugin for IDE integration to format the source file while editing it

For my setup that works fine, I use the Spotless Applier plugin for Intellij but there is one for VSCode as well afaict.

The workaround for the closing braces fix will hopefully go away soon.

[netomi]

For me this looks really good now, the majority of the changes is for the closing braces fix, which will hopefully go away. If this also looks good for others we can integrate, then apply to the whole codebase in a separate PR and add the format to the pre-commit hook.

[cstamas]

Looks good!

[autumnfound]

There's some funkiness around nested chaining that imo really doesn't look good. I'm thinking that this is due to the closing brace code that was added being a little too eager.

Example from AdminAPITest

        mockMvc.perform(
                post(
                        "/admin/namespace/{namespace}/change-member?user={user}&role={role}",
                        "foobar",
                        "other_user",
                        "remove"
                )
                        .with(user("admin_user").authorities(new SimpleGrantedAuthority(("ROLE_ADMIN"))))
                        .with(csrf().asHeader())
        )
                .andExpect(status().isOk())
                .andExpect(content().json(successJson("Removed other_user from namespace foobar.")));

Do we want to be running braceless 1 line ifs? I'm personally not a huge fan of it in general, though I think the old way is better than the new in this case.

new

        if (this == o)
            return true;
        if (o == null || getClass() != o.getClass())
            return false;

old

        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;

Also not a huge fan of mixing 4 and 8 space indenting, but that seems to be an intentional choice here.

[netomi]

the closing brace fix is for array initializers only, I noticed this verbose formatting in some cases as well. This is the 1% I am not happy with, I can still play around with it to have it more compact though.

I would prefer braces to always be present in control structures, but the existing code omits them in many places so I kept it like that for now. We might wanna change that in a second run though.

[netomi]

the mixing of 4 and 8 spaces is due to continuation rules. The default seems to be 2x of the normal indentation. Thats also what the code uses now most of the time. As always you find cases where 4 looks better, in other 8, so I am split here.

At some point I tried a continuation of 4, but that also looked ugly in some occasions imho.

[brianking]

Might be a good idea to standardize / document these coding rules across the whole codebase?

[autumnfound]

I would prefer braces to always be present in control structures, but the existing code omits them in many places so I kept it like that for now. We might wanna change that in a second run though.

+1 to using explicit braces even when optional. Is there a point to doing a second sweeping set of changes if we're going to be doing something that already touches a large portion of the codebase as it is though?

At some point I tried a continuation of 4, but that also looked ugly in some occasions imho.

Fair enough! It might just be because we're enforcing it so consistently and in the past we had cases where the continuation was 4 spaces that it's noticable.

[cstamas]

Yes, braces should be there always, is superb for readability.

[netomi]

the braces stuff will require a step like for the import order as the eclipse formatter does not handle that, but claude is busy coming up with something.

@brianking yeah the idea is to document that. So far code did not follow any styleguide so we are changing that. For the frontend we already use prettier.

[netomi]

• braces are now added
• chained method execution looks now more sane -> the downside is I had to remove that the closing bracket of a method invocation is on a new line if wrapped, but that is acceptable imho
• added already a pre-commit hook to check that this works reliable, this PR has ofc many violations as the source is not yet fixed