Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic Contribution Requirements #13754

Open
codeconsole opened this issue Oct 14, 2024 · 8 comments
Open

Basic Contribution Requirements #13754

codeconsole opened this issue Oct 14, 2024 · 8 comments

Comments

@codeconsole
Copy link
Contributor

codeconsole commented Oct 14, 2024

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

@codeconsole codeconsole moved this to Todo in Grails 7 Oct 14, 2024
@amondel2
Copy link

amondel2 commented Oct 18, 2024

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
image

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
Copy link
Contributor Author

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
Copy link

amondel2 commented Oct 19, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

codeconsole commented Oct 24, 2024

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
Copy link

amondel2 commented Nov 3, 2024

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
Copy link
Contributor

jamesfredley commented Nov 7, 2024

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

@erickinsella
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

No branches or pull requests

4 participants