Basic Contribution Requirements

[codeconsole]

Code Format

In the Grails Core module:

if()

occurs 1529 times vs

if (

occurs 6502 times.

I always remember back in the day at the University, my professor would say... if is not a function; therefore, it should be formatted accordingly.

Code formatting should not be left to personal preference. Fortunately, a great deal of time and effort has been invested by others to establish good practices on how to properly format code, enhancing its readability and promoting consistency throughout the codebase.

Some good starting points are: https://www.oracle.com/java/technologies/javase/codeconventions-contents.html (With the exception of line wrapping as screen widths have changed)

https://google.github.io/styleguide/javaguide.html](https://google.github.io/styleguide/javaguide.html

I recommend we start with consistent bracing and horizontal whitespace

[amondel2]

I created the following configuration file to have a more concrete discussion on what settings we should use, I think we can create three different exports. Their was a proposal to add .editorconfig to start standardizing on a consistent code style to help with the diffs on merging. However, when I looked at the import options in IntelliJ I only saw the following

Generally I go with removing spaces, but I think braces really help with readability. In a bunch of places I proposed only wrapping if long, but I think we need some discussion on what format makes the most sense.

Please note the txt version and XML should be the same and they were modified so it could be uploaded to github.
Grails_Style_Prop.xml.zip Grails_Style_Prop_AS_TXT.txt

[codeconsole]

Can you define "removing spaces", are you suggesting tabs?

Since we are heavily committed to Spring, I recommend we also align our code style equivalently. Spring Framework Code-Style

These standards are heavily adopted by Spring, Google, and are also the standard for the programming in the Java Language.

Those specifications require braces to be K&R style. And proper spacing around any reserved word.

[amondel2]

I am not suggesting tabs; I meant spaces in between parentheses. However, I don't have a strong opinion on this, other than it being consistent across the codebase. The only code style I have a strong preference for is not putting else, catch, etc., on a new line but rather keeping them on the same line with a brace, as demonstrated in section 4.1.2 Nonempty blocks: K & R style . This seems different from what is in the Spring Framework Code Style, so I am fine setting up the configuration that aligns with the Spring style, as long as the rest of the community agrees that it is the best path forward.

[jamesfredley]

Examples of Grails and adjacent projects which are enforcing style rules:

grails-forge uses checkstyle: https://github.com/grails/grails-forge/tree/7.0.x/config/checkstyle

webdriver-binaries-gradle-plugin uses codenarc: https://github.com/erdi/webdriver-binaries-gradle-plugin/tree/master/gradle/codenarc

[codeconsole]

Spring Security automates the entire process and has a format gradle plugin where it will automatically format your code according to their specifications so not changes are actually required by the developer.

If they do not format according to spec, the build fails and the PR is rejected.

We should just attempt to use the same plugin and see if it formatting works for Groovy and it is acceptable.

They also have a required IntelliJ plugin. "Plugin 'CheckStyle-IDEA' required for 'spring-security' project isn't installed."

[amondel2]

I finally had a few minutes to get back to this. The plugin that @codeconsole mentioned in the previous post is a tool for formatting Java code. It’s called spring-javaformat and is developed by the Spring team.

Someone asked about Groovy support, but it looks like they don’t plan to add it.

I also came across another promising tool: Spotless. I might try it out. If anyone is currently using something for Groovy formatting, I’d love to hear your recommendations!

[jamesfredley]

https://github.com/grails/grails-forge/tree/7.0.x/config is also using spotless, currently.

[erickinsella]

I also agree that having a set of formatting standards is more important than their specificity. An easy way to automate the standards when committing would be "a nice to have" IMO.

During a recent Grails Planning meeting there was a discussion about whether formatting would happen all at once per repo or as files changed. I'm not sure where that discussion ended up.

[dakshmehta007]

@codeconsole I have a solution for this issue. May I raise the PR?

[jdaugherty]

@dakshmehta007 You can open a PR and we can use the PR to discuss your solution.

[borinquenkid]

If we use Spotless we do not only check for standards alignment but also fix it.

Use in gradle

[jamesfredley]

I have had a good experience with spotless and checkstyle, which are in place on https://github.com/apache/grails-forge

https://github.com/search?q=repo%3Aapache%2Fgrails-forge%20spotless&type=code https://github.com/search?q=repo%3Aapache%2Fgrails-forge+checkstyle&type=code

spotless appears to be a better fit than io.spring.javaformat, used by Spring Boot, due to groovy support.

[jamesfredley]

@amondel2 @borinquenkid @erickinsella @dakshmehta007

As part of the Apache transition we have had to add license headers to most of the files which causes each file to have a recent change in git history.

Reformatting the code will obviously change a lot more than that in each file, but this may be a good time to revisit this.

Plus nearly all the code is now in https://github.com/apache/grails-core/

[amondel2]

I haven’t used Spotless before, but based on a quick look, it seems like a good option. I was originally thinking that, due to the number of changes this would cause, it would make sense to treat it as a minor release. However, the main concern that this change would make it difficult to diff with a previous version is no longer really valid given the amount of changes already made.

@jamesfredley are you thinking that we should do the changes and then make this part of the CI that runs on any pull request?

[jdaugherty]

@amondel2 Yes.

[jamesfredley]

grails-forge enforces checkstyle and spotless on build and CI. It's config might be worth reviewing, although grails-core can be configured however makes the most sense.

https://github.com/search?q=repo%3Aapache%2Fgrails-forge+checkstyle&type=code https://github.com/search?q=repo%3Aapache%2Fgrails-forge%20spotless&type=code

[jamesfredley]

Timing wise, we should have two more projects merged into grails-core (grails-wrapper and grails-profiles) and have license headers wrapped up next week and then it would be good to start looking at this.

[jdaugherty]

FYI: Git supports 'ignoring a revision' when using git blame and can be configured by default to do so. If we reformat the code in one change, we can add that revision to be ignored:

   blame.ignoreRevsFile
   Ignore revisions listed in the file, one unabbreviated object name per line, in git-blame[1]. Whitespace and comments beginning with # are ignored. This option may be repeated multiple times. Empty file names will reset the list of ignored revisions. This option will be handled before the command line option --ignore-revs-file.

[jdaugherty]

I've made a pass at reformatting all of the non-test code under the grails-project project in https://github.com/apache/grails-core/pull/14903 Please note this also applies the code style from grails-forge.

[jamesfredley]

New PR: https://github.com/apache/grails-core/pull/14925