Optional automated source code formatting - prep for JEP

592 views
Skip to first unread message

Mark Waite

unread,
Jul 23, 2018, 9:36:42 PM7/23/18
to jenkinsci-dev
I'd like to submit a Jenkins Enhancement Proposal for optional automated source code formatting available from the plugin pom so that plugins which build with maven could choose to "opt-in" to automated source code formatting.

Key attributes of the idea:
  • Optional - plugins only get source code formatting if they enable it
  • Automatic - when source code formatting is enabled, it happens automatically as part of the maven compile phase
  • Common - when source code formatting is enabled, the source code formatting settings are not intended to be altered by the plugin
I've implemented a sample in the platformlabeler plugin using the "FMT Maven plugin" which uses google-java-format to format Java source code to Google Java Style.

Key questions that need discussion (especially considering my feeble skills with maven):
  • Is optional automated source formatting of interest to a few plugin developers that would be willing to test drive it?  If so, who are the plugin developers that are willing to test drive it, and which plugins will be involved in the test drive?  I'll use platformlabeler as one of the test drive plugins.  Others will be needed
  • How would a prototype of optional automated source formatting be evaluated?  By releasing a beta version of the plugin parent pom?  Some other means?
  • Would automated formatting be integrated into the plugin parent pom using techniques similar to the findbugs integration into the plugin parent pom, or is there a better way?
  • Is "google java format" an acceptable code format for those plugin authors that are willing to adopt automated code formatting?
  • Are there other issues that need to be addressed before a Jenkins Enhancement Proposal is submitted for optional automated source code formatting?
A plugin maintainer that chooses to adopt automated source code formatting needs to understand that automated source code formatting creates a "diff wall" due to the many changes made by the formatting process.  That is one of the penalties associated with automated source code formatting.

Plugins with many open pull requests (git plugin, git client plugin) should not enable automated formatting until they've reduced the number of open pull requests to very few.  Otherwise, most open pull requests for the plugin will have many conflicts as soon as the automated formatting is committed.  That is another of the penalties of automated source code formatting.

Plugins that adopt automated source code formatting will likely find it easier to read the source code and easier to submit clean pull requests.  Automated formatting avoids the poor experience of pull request submitters being asked to fix the white space diffs introduced by their pull request.

Thanks,
Mark Waite

Joseph P

unread,
Jul 24, 2018, 3:48:04 AM7/24/18
to Jenkins Developers
The problem I have noticed with FMT Maven plugin. The formatter does not handle CR LF line endings rather it converts everything to LF which will mess with commits because it causes high rewrites. So you would need to ensure that git attributes is in all repos as well to avoid incorrect commit history.

Robert Sandell

unread,
Jul 24, 2018, 6:31:31 AM7/24/18
to jenkin...@googlegroups.com
I am more of the philosophy of "teach a man to code and he will eat for a lifetime" approach. So most of my "originally authored" plugins have checkstyle rules applied to the build.
It is by no means perfect, but I haven't had to complain about white spaces in PRs to those plugins in a long time :)

/B

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/c3ef8d20-8332-47f6-9803-68178421aac0%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


--
Robert Sandell
Software Engineer
CloudBees, Inc.
CloudBees-Logo.png
Twitter: robert_sandell

Mark Waite

unread,
Jul 24, 2018, 8:30:01 AM7/24/18
to jenkin...@googlegroups.com
On Tue, Jul 24, 2018 at 1:48 AM Joseph P <jose...@gmail.com> wrote:
The problem I have noticed with FMT Maven plugin. The formatter does not handle CR LF line endings rather it converts everything to LF which will mess with commits because it causes high rewrites. So you would need to ensure that git attributes is in all repos as well to avoid incorrect commit history.



I like the idea of including a .gitattributes file as part of the transition to optional automatic formatting in a repository.  I'll include that in the proposal.  Additional files were needed with the transition to support incrementals and that transition seems to be progressing very well.

I prefer (and accept) the intentional setting of line endings.  I consider that part of the standardization of the formatting.  I think that maintainers that don't want line endings standardized or that cannot accept LF as the standard would choose to not enable automatic formatting in the plugins they maintain.

Thanks,
Mark Waite
 

tirsdag den 24. juli 2018 kl. 03.36.42 UTC+2 skrev Mark Waite:
I'd like to submit a Jenkins Enhancement Proposal for optional automated source code formatting available from the plugin pom so that plugins which build with maven could choose to "opt-in" to automated source code formatting.

Key attributes of the idea:
  • Optional - plugins only get source code formatting if they enable it
  • Automatic - when source code formatting is enabled, it happens automatically as part of the maven compile phase
  • Common - when source code formatting is enabled, the source code formatting settings are not intended to be altered by the plugin
I've implemented a sample in the platformlabeler plugin using the "FMT Maven plugin" which uses google-java-format to format Java source code to Google Java Style.

Key questions that need discussion (especially considering my feeble skills with maven):
  • Is optional automated source formatting of interest to a few plugin developers that would be willing to test drive it?  If so, who are the plugin developers that are willing to test drive it, and which plugins will be involved in the test drive?  I'll use platformlabeler as one of the test drive plugins.  Others will be needed
  • How would a prototype of optional automated source formatting be evaluated?  By releasing a beta version of the plugin parent pom?  Some other means?
  • Would automated formatting be integrated into the plugin parent pom using techniques similar to the findbugs integration into the plugin parent pom, or is there a better way?
  • Is "google java format" an acceptable code format for those plugin authors that are willing to adopt automated code formatting?
  • Are there other issues that need to be addressed before a Jenkins Enhancement Proposal is submitted for optional automated source code formatting?
A plugin maintainer that chooses to adopt automated source code formatting needs to understand that automated source code formatting creates a "diff wall" due to the many changes made by the formatting process.  That is one of the penalties associated with automated source code formatting.

Plugins with many open pull requests (git plugin, git client plugin) should not enable automated formatting until they've reduced the number of open pull requests to very few.  Otherwise, most open pull requests for the plugin will have many conflicts as soon as the automated formatting is committed.  That is another of the penalties of automated source code formatting.

Plugins that adopt automated source code formatting will likely find it easier to read the source code and easier to submit clean pull requests.  Automated formatting avoids the poor experience of pull request submitters being asked to fix the white space diffs introduced by their pull request.

Thanks,
Mark Waite

--

Jesse Glick

unread,
Jul 24, 2018, 8:30:52 AM7/24/18
to Jenkins Dev
On Mon, Jul 23, 2018 at 9:36 PM Mark Waite <mark.ea...@gmail.com> wrote:
> Automatic - when source code formatting is enabled, it happens automatically as part of the maven compile phase

Meaning that `mvn compile` modifies `src/main/java/**/*.java`?
Convenient if potentially surprising.

But what about pull requests in which the author never ran Maven to
this goal, and edited sources do not match the format—is there any way
for the PR build to fail in this case? (Note that this does _not_ mean
the PR author never tested/tried the code. Some IDEs run internal
compilation, bypassing Maven.)

> How would a prototype of optional automated source formatting be evaluated? By releasing a beta version of the plugin parent pom?

Just by doing a regular release of the plugin parent POM. If the
change is a dud, we just back that configuration out in a later
release.

> Is "google java format" an acceptable code format for those plugin authors that are willing to adopt automated code formatting?

BTW I am leery of automated source formatting generally, but if I have
to comply with a format, it is nice that it allows inline
determinative annotations such as `@Override` or `@Test` without
someone yelling at me about some arbitrary Sun Microsystems-era
guideline. :-) It remains unfortunate that it prohibits inline
annotations with any parameters even when they are determiners, like
the second line in

@Parameterized.Parameter public String x;
@Parameterized.Parameters(name = "x={0}") public static String[] allowedXs() {
// …
}

since inserting a line break would make `Parameters` look like a mere
attribute, which it is not.

My heart was also warmed by this note:

> Never make your code less readable simply out of fear that some programs might not handle non-ASCII characters properly. If that should happen, those programs are broken and they must be fixed.

Mark Waite

unread,
Jul 24, 2018, 8:37:46 AM7/24/18
to jenkin...@googlegroups.com
On Tue, Jul 24, 2018 at 4:31 AM Robert Sandell <rsan...@cloudbees.com> wrote:
I am more of the philosophy of "teach a man to code and he will eat for a lifetime" approach. So most of my "originally authored" plugins have checkstyle rules applied to the build.
It is by no means perfect, but I haven't had to complain about white spaces in PRs to those plugins in a long time :)

/B


I like that as an alternative for consideration.  My experience with checkstyle has not been as positive as yours.  I'll try it with platformlabeler plugin to see if I can find settings that work for me.

At a minimum, I'll include in an eventual JEP a suggestion that checkstyle is an alternative to source code formatting, since it will check the formatting without modifying the formatting.

Mark Waite
 

Baptiste Mathus

unread,
Jul 24, 2018, 9:00:32 AM7/24/18
to Jenkins Developers
Great idea and initiative IMO, thanks a bunch for driving this Mark. 

Over the years, I've become more and more strongly opinionated that *a* common format is defined, and easily applied, and less and less about the precise format.
IOW, I generally do have opinions about where curly brackets and others should go, but I'll happily put them aside if this helps us define *something*.

https://github.com/jenkinsci/radiatorview-plugin might also be interesting as it has a bit more code, and has been existing for quite long I think.

-- Baptiste



--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

Liam Newman

unread,
Jul 24, 2018, 4:44:25 PM7/24/18
to jenkin...@googlegroups.com
The auto format should occur before the compile stage, probably during process-sources.

Mark Waite

unread,
Jul 24, 2018, 6:21:00 PM7/24/18
to jenkin...@googlegroups.com
On Tue, Jul 24, 2018 at 6:30 AM Jesse Glick <jgl...@cloudbees.com> wrote:
On Mon, Jul 23, 2018 at 9:36 PM Mark Waite <mark.ea...@gmail.com> wrote:
> Automatic - when source code formatting is enabled, it happens automatically as part of the maven compile phase

Meaning that `mvn compile` modifies `src/main/java/**/*.java`?
Convenient if potentially surprising.

That was my intent.  Based on the later comment from Liam Newman, I'd change the proposal to optionally insert the fmt-mvn-plugin into the process-sources phase (before compile).
 
But what about pull requests in which the author never ran Maven to
this goal, and edited sources do not match the format—is there any way
for the PR build to fail in this case? (Note that this does _not_ mean
the PR author never tested/tried the code. Some IDEs run internal
compilation, bypassing Maven.)

That's an excellent point.  Do you think it would be acceptable to fail the process-sources phase (or the compile phase) if it detected that we were running inside Jenkins and a file had been modified at the end of the process-sources phase?

Is there a reasonable way to do that in maven?

The fmt-mvn-plugin has a "check" goal which will report files which would be modified without modifying them.  I assume that could be used in an optional step in the Jenkinsfile to decide if a pull request was not formatted to match the settings of that plugin.
 
> How would a prototype of optional automated source formatting be evaluated?  By releasing a beta version of the plugin parent pom?

Just by doing a regular release of the plugin parent POM. If the
change is a dud, we just back that configuration out in a later
release.

> Is "google java format" an acceptable code format for those plugin authors that are willing to adopt automated code formatting?

BTW I am leery of automated source formatting generally, but if I have
to comply with a format, it is nice that it allows inline
determinative annotations such as `@Override` or `@Test` without
someone yelling at me about some arbitrary Sun Microsystems-era
guideline. :-) It remains unfortunate that it prohibits inline
annotations with any parameters even when they are determiners, like
the second line in

@Parameterized.Parameter public String x;
@Parameterized.Parameters(name = "x={0}") public static String[] allowedXs() {
    // …
}

since inserting a line break would make `Parameters` look like a mere
attribute, which it is not.


I'm not sure how to address this specific concern.  Any suggestions?

Mark Waite
 
My heart was also warmed by this note:

> Never make your code less readable simply out of fear that some programs might not handle non-ASCII characters properly. If that should happen, those programs are broken and they must be fixed.

 
--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

Jesse Glick

unread,
Jul 24, 2018, 6:52:36 PM7/24/18
to Jenkins Dev
On Tue, Jul 24, 2018 at 6:21 PM Mark Waite <mark.ea...@gmail.com> wrote:
> Do you think it would be acceptable to fail the process-sources phase (or the compile phase) if it detected that we were running inside Jenkins and a file had been modified at the end of the process-sources phase?

Would work. We actually do something similar in core:

https://github.com/jenkinsci/jenkins/blob/1a11fa39b567fd7ff64e61fab23aa18a123f2d97/Jenkinsfile#L48

> The fmt-mvn-plugin has a "check" goal which will report files which would be modified without modifying them.

…and fails if so:

https://github.com/coveo/fmt-maven-plugin/blob/145bbfbc0e9eb6f2aaf8b6bdf930efef5a8ddbef/src/main/java/com/coveo/Check.java#L63

> I assume that could be used in an optional step in the Jenkinsfile

You could put this into a Maven profile and activate it from
`buildPlugin.groovy`.

> I'm not sure how to address this specific concern.

Not a concern; you may disregard.

Robert Sandell

unread,
Jul 25, 2018, 5:14:00 AM7/25/18
to jenkin...@googlegroups.com
Might also need to turn it off during "mvn release". It would be very strange, and probably bad, if the -sources.jar didn't match the sources on GitHub for the release tag. Or maybe that just gets pushed together with the "[maven-release-plugin] prepare release ..." commit?

/B

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

Joseph P

unread,
Jul 25, 2018, 6:44:48 AM7/25/18
to Jenkins Developers
Just to touch on the line endings issue... See https://github.com/jenkinsci/vstestrunner-plugin/pull/16 when it becomes an issue. I just wanted to fix a minor thing but because of the maintainer did a commit where they messed with line endings it screwed up the history for https://github.com/jenkinsci/vstestrunner-plugin/pull/17

Joseph P

unread,
Jul 25, 2018, 7:16:54 AM7/25/18
to Jenkins Developers
Or maybe that just gets pushed together with the "[maven-release-plugin] prepare release ..." commit?

I see no problem in pushing on prepare release commit 😅

Mark Waite

unread,
Jul 25, 2018, 8:06:31 AM7/25/18
to jenkin...@googlegroups.com
On Wed, Jul 25, 2018 at 4:44 AM Joseph P <jose...@gmail.com> wrote:
Just to touch on the line endings issue... See https://github.com/jenkinsci/vstestrunner-plugin/pull/16 when it becomes an issue. I just wanted to fix a minor thing but because of the maintainer did a commit where they messed with line endings it screwed up the history for https://github.com/jenkinsci/vstestrunner-plugin/pull/17


I see the changes in line endings between those two commits but am not clear on the actions which are needed by a maintainer to avoid the problem.

Is it enough to have the .gitattributes file that sets the line endings of files that will be automatically formatted?

If that's not enough, could you explain further what is needed?

Mark Waite
 

Joseph P

unread,
Jul 25, 2018, 8:26:22 AM7/25/18
to Jenkins Developers
The action needed to avoid line endings being messed with is 
* text=auto

However this would be enough for the formatter
*.java text eol=lf

But I would recommend the text=auto

And if the maintainer needs something more fancy they can customise it further but lucky shouldn't have anything like Visual Studio
*.sln        text eol=crlf

If a maintainer mess it up the answer would be to add the .gitattributes and run 
git add --renormalize .

Like I did in the first PR.

Tomas Bjerre

unread,
Jul 25, 2018, 9:28:58 AM7/25/18
to Jenkins Developers
This is great stuff! Thanks Mark =)

I've been doing this in my plugins for a couple of years now and I have only nice things to say. It helps contributors a lot as well as they can focus on the implementation and not fiddle with formatting.

Jesse Glick

unread,
Jul 25, 2018, 9:30:12 AM7/25/18
to Jenkins Dev
On Wed, Jul 25, 2018 at 5:14 AM Robert Sandell <rsan...@cloudbees.com> wrote:
> Might also need to turn it off during "mvn release".

And Incrementals. This is feeling increasingly scary.

Generally speaking, I would find it an unwelcome surprise if a `mvn
install` or whatever _modified_ my source tree. It is OK for some
plugin to cause `validate` or `prepare-sources` or some such phase to
fail when sources are not in compliance with some standard (so long as
this check is fast enough to not burden a developer workflow, or is
suppressed during some sort of quick build profile TBD)—but at most
such a mojo should print a failure message to the effect of

> Source formatting out of compliance. To fix, run: mvn fmt:format

so that the change to sources is a conscious decision, one I know I
need to commit (perhaps `--amend`) and include in my PR.

Mark Waite

unread,
Jul 25, 2018, 9:40:03 AM7/25/18
to jenkin...@googlegroups.com
That's a good insight.  I prefer that if the maintainer has chosen to enable automatic source formatting that it is "always on".  I think your observation means that you would probably not choose to enable automatic source formatting in the plugins you maintain if the formatting is "always on".

Would it meet your needs if there were two modes of operation, one that includes a fmt-maven-plugin:check in the process-sources phase without modifying source code, and one that performs the formatting in the process-sources phase?

If that were the path we followed, then I would assume that the new argument added to buildPlugin for ci.jenkins.io would perform a fmt-maven-plugin:check if formatting were enabled for that plugin.

Mark Waite
 
--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

Jesse Glick

unread,
Jul 25, 2018, 11:21:25 AM7/25/18
to Jenkins Dev
On Wed, Jul 25, 2018 at 9:40 AM Mark Waite <mark.ea...@gmail.com> wrote:
> I think your observation means that you would probably not choose to enable automatic source formatting in the plugins you maintain if the formatting is "always on".

No, I am referring to _contributing_ to such a plugin maintained by
someone else. I would not expect a `mvn` command in the default
lifecycle to ever modify a versioned file. That generally counts as a
serious bug in the build setup.

> Would it meet your needs if there were two modes of operation, one that includes a fmt-maven-plugin:check in the process-sources phase without modifying source code, and one that performs the formatting in the process-sources phase?

There are already two modes of operation being discussed. The question
is what happens by default when I clone somebody’s repo, edit a couple
of lines, run tests from my IDE, commit, maybe do some follow-up Maven
build, and file a PR against it. Am I suddenly left with uncommitted
modifications in my source tree?

Worse, what happens when I edit some lines, instruct my IDE to run a
Maven build to run tests (saving all modified files in the process),
then start making some follow-up edits to sources while I am waiting
for Maven to finish because I realize I forgot something? I am going
to get a conflict and a confusing dialog in my IDE asking me to decide
whether to save my actual edits, or discard and reload from disk.

Perhaps the reformatting during a default lifecycle phase could be
something that you (the developer working locally, not the plugin
maintainer!) could explicitly opt in to, for example with a local
profile in `~/.m2/settings.xml`. Then you are at least expecting this
to happen and are making corresponding accommodations in your
development workflow.

Mark Waite

unread,
Jul 25, 2018, 12:04:11 PM7/25/18
to jenkin...@googlegroups.com
On Wed, Jul 25, 2018 at 9:21 AM Jesse Glick <jgl...@cloudbees.com> wrote:
On Wed, Jul 25, 2018 at 9:40 AM Mark Waite <mark.ea...@gmail.com> wrote:
> I think your observation means that you would probably not choose to enable automatic source formatting in the plugins you maintain if the formatting is "always on".

No, I am referring to _contributing_ to such a plugin maintained by
someone else. I would not expect a `mvn` command in the default
lifecycle to ever modify a versioned file. That generally counts as a
serious bug in the build setup.


Thanks for the clarification.  I hadn't seen that as a serious bug in the build setup.  That changes how I was intending to approach the problem.

I think that means that if optional source formatting is enabled, then the process-sources phase would execute fmt-maven-plugin:check and fail the build if formatting changes were needed. The developer would then run `mvn fmt-maven-plugin:format` to format the files, then return to running their usual phase (`mvn install` or `mvn verify` or ...).
 
> Would it meet your needs if there were two modes of operation, one that includes a fmt-maven-plugin:check in the process-sources phase without modifying source code, and one that performs the formatting in the process-sources phase?

There are already two modes of operation being discussed. The question
is what happens by default when I clone somebody’s repo, edit a couple
of lines, run tests from my IDE, commit, maybe do some follow-up Maven
build, and file a PR against it. Am I suddenly left with uncommitted
modifications in my source tree?

Worse, what happens when I edit some lines, instruct my IDE to run a
Maven build to run tests (saving all modified files in the process),
then start making some follow-up edits to sources while I am waiting
for Maven to finish because I realize I forgot something? I am going
to get a conflict and a confusing dialog in my IDE asking me to decide
whether to save my actual edits, or discard and reload from disk.

Perhaps the reformatting during a default lifecycle phase could be
something that you (the developer working locally, not the plugin
maintainer!) could explicitly opt in to, for example with a local
profile in `~/.m2/settings.xml`. Then you are at least expecting this
to happen and are making corresponding accommodations in your
development workflow.


I like that idea.  The plugin maintainer chooses to have source formatting checked as part of a default phase.  The plugin developer may either perform the required source formatting as a separate maven command or may choose to configure their local development environment with a local profile in ~/.m2/settings.xml.
 
--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

Jesse Glick

unread,
Jul 25, 2018, 2:13:51 PM7/25/18
to Jenkins Dev
On Wed, Jul 25, 2018 at 12:04 PM Mark Waite <mark.ea...@gmail.com> wrote:
> if optional source formatting is enabled, then the process-sources phase would execute fmt-maven-plugin:check and fail the build if formatting changes were needed.

(with a helpful message, I hope)

> The developer would then run `mvn fmt-maven-plugin:format`

(unless the `fmt` plugin group prefix were made available)

> to format the files

Or choose to use an IDE configuration that does this in the editor.

> then return to running their usual phase (`mvn install` or `mvn verify` or ...).

Exactly.

>> Perhaps the reformatting during a default lifecycle phase could be
>> something that you […] could explicitly opt in to
>
> I like that idea. The plugin maintainer chooses to have source formatting checked as part of a default phase. The plugin developer may either perform the required source formatting as a separate maven command or may choose to configure their local development environment […]

Right.

Joseph P

unread,
Aug 20, 2018, 10:39:46 AM8/20/18
to Jenkins Developers
I just realized this is the minimum

*.java eol=lf

However this would be could to have in all repos:
* text=auto
*.java eol=lf

Jesse Glick

unread,
Feb 22, 2021, 4:35:21 PM2/22/21
to Jenkins Dev

Baptiste Mathus

unread,
Feb 23, 2021, 9:00:05 AM2/23/21
to Jenkins Developers
Thinking about this more, I agree the right phase should be on both process-resources and process-test-resources respectively (though a bit surprising for users indeed).

On the way it should operate: I was going to propose a CI-only flag to fail there when a format is not good, but to avoid the CI surprise, I'm thinking the standard build should:
* Detect a formatting change need, do it, and non-zero exit
Or
* Detect no formatting need and do nothing

This way the behavior is the same locally and in Jenkins, and it should not be too disruptive as it's not very late in the Maven lifecycle.

I think non-zero exiting is useful locally, so you don't end up git add & commit, mvn test, git push but then get a CI failure because you had a local formatting change you didn't add before pushing.

Le mar. 24 juil. 2018 à 22:44, Liam Newman <bitwi...@gmail.com> a écrit :
The auto format should occur before the compile stage, probably during process-sources


jn...@cloudbees.com

unread,
Feb 24, 2021, 6:32:23 AM2/24/21
to Jenkins Developers
Hi all,

So I have some strong opinions here and these are that nothing should ever be auto applied.

Whatever format you have defined it will produce worse code in some situations, and this results in code littered with //AUTO_FORMAT_IGNORE:next_10_lines which does not aid to the readability of the code or code that is just harder to read than it should be.

I have found that what works best is if there is a way for users to format my changes "on demand" by say running a specific maven goal, or having a specific IDE configuration (not all IDEs can format the same way so this last one is hard) , and for the CI to flag deviations as part of the code review.  ie it should not fail the build, but say "this line violates rule line_length > X".  That way as a maintainer I can choose to accept something that I believe is legible when the committer has left a comment saying why the code deviates, or just leave it as the CI saying "please fix your style issues".

There is also nothing worse than running `mvn -Dmaven-surefire-debug=XYZ test -Dtest-foobar` and having all the breakpoints messed up because the source code changes underneath you, or running tests from your IDE, committing and then pushing only for the CI to fail with something that is probably not easy to see (exit code 1 without any CI check comment?)

What I feel is also missing here is what issue is this attempting to solve.  It is proposing a solution - but what is the exact problem maintainers are seeing; perhaps there are better ways to solve that?

/James







Mark Waite

unread,
Feb 24, 2021, 5:53:48 PM2/24/21
to Jenkins Developers
On Wednesday, February 24, 2021 at 4:32:23 AM UTC-7 James wrote:
Hi all,

So I have some strong opinions here and these are that nothing should ever be auto applied.

Whatever format you have defined it will produce worse code in some situations, and this results in code littered with //AUTO_FORMAT_IGNORE:next_10_lines which does not aid to the readability of the code or code that is just harder to read than it should be.

I have found that what works best is if there is a way for users to format my changes "on demand" by say running a specific maven goal, or having a specific IDE configuration (not all IDEs can format the same way so this last one is hard) , and for the CI to flag deviations as part of the code review.  ie it should not fail the build, but say "this line violates rule line_length > X".  That way as a maintainer I can choose to accept something that I believe is legible when the committer has left a comment saying why the code deviates, or just leave it as the CI saying "please fix your style issues".

There is also nothing worse than running `mvn -Dmaven-surefire-debug=XYZ test -Dtest-foobar` and having all the breakpoints messed up because the source code changes underneath you, or running tests from your IDE, committing and then pushing only for the CI to fail with something that is probably not easy to see (exit code 1 without any CI check comment?)

What I feel is also missing here is what issue is this attempting to solve.  It is proposing a solution - but what is the exact problem maintainers are seeing; perhaps there are better ways to solve that?


I was trying to avoid changes in indentation, braces, and comments that I felt did not add value to git plugin and git client plugin pull requests.  I was spending time reminding submitters that the contributing guide in those plugins specifically asked that they not make white space changes without consulting the maintainers.  I was occasionally frustrated when I failed to follow the contributing guidelines for that plugin in my reviews.  Then white space changes would arrive in the code even though the guidelines said they should not.  Worrying about formatting during code review seemed like a waste of time.  Two years ago the git plugin had over 60 pull requests open proposing various changes.  A white space or formatting change in one of those pull requests would cause another pull request to have a conflict.

Since that time, I've resolved the issue in the git plugin another way.  I declared pull request review bankruptcy and closed 50+ pull requests because I was clearly not making progress reviewing them.

Since then, I've implemented mandatory code formatting in another plugin (platformlabeler) as an experiment and have found the code formatting helpful.  The one open pull request on that repository has been complicated by a "diff wall" created by a change from two space indentation to four space indentation, but that diff wall was not difficult to handle in that case.

I still remember with affection the experience watching a team adopt automated formatting of their Java code just because Kent Beck's Extreme Programming book said to do it.  We were feeling bold and wanted to go "all in".  It removed one area of friction and allowed them to work better together.  I suspect the `go fmt` command is similarly used to reduce friction due to formatting styles.

I've accepted that the git plugin and git client plugin will likely never use automated formatting because of the "diff wall" it would create.  Others have described techniques to avoid or manage the "diff wall", but the formatting in those two plugins is such a mess that I believe the "diff wall" is unavoidable in their cases.

Mark Waite

Basil Crow

unread,
Feb 24, 2021, 7:10:26 PM2/24/21
to jenkin...@googlegroups.com
On Wed, Feb 24, 2021 at 3:32 AM jn...@cloudbees.com <jn...@cloudbees.com> wrote:
>
> What I feel is also missing here is what issue is this attempting to solve.
> It is proposing a solution - but what is the exact problem maintainers are
> seeing; perhaps there are better ways to solve that?

The problem, to me, is twofold:

1. Lack of convenient formatting tools means one has to consciously think
about formatting when writing new code or changing existing code. This
slows down the development process.
2. Lack of a consistently enforced formatting style throughout a
repository leads to PRs that change both logic and formatting style.
This slows down the review process.

Over the years I have led a number of efforts to reformat legacy codebases
and enforce the new formatting style consistently at build time, both in
Java (using Google Java Format) and Python (using Black). My experience is
that while the "diff wall" described by Mark does initially lead to some
frustration, the long term benefits of a consistently enforced formatting
style eventually outweigh the initial costs. In my experience, the key
takeaways are:

1. Choose a formatting tool that exposes extremely few (if any)
configuration options and that formats the majority of code at high
quality.
2. Provide developers with a single command (not executed by default) to
automatically format their code prior to committing it (ideally also
with IDE integration).
3. Enforce that the entire repository is consistently formatted during the
build system's "verify" phase (Maven) or "check" task (Gradle), but do
not change any existing code as part of this process.

This solves the originally mentioned problem as follows:

1. A formatting tool that is integrated with the build system means that
one no longer has to consciously think about formatting when writing
new code or changing existing code. This speeds up the development
process.
2. A consistently enforced formatting style throughout a repository
eliminates PRs that change both logic and formatting style. There is
only one formatting style, and that is the style imposed by the
formatting tool during the build (otherwise the build is unstable).
This speeds up the review process.

Some practical notes:

1. The Eclipse JDT formatter is very customizable, but customizability is
a non-goal when trying to enforce a consistent formatting style across
an entire repository.
2. Google Java Format [1] and Black [2] are intentionally not customizable
and format the majority of code at high quality.
3. One exception to the above with Google Java Format is that it supports
an "AOSP mode" to switch from 2-space indentation to 4-space
indentation. One might be tempted to use this mode when formatting a
legacy codebase that already uses 4-space indentation; however, the
result is often unreadable lambda expressions [3], and the maintainers
do not appear to be willing to address this. I highly recommend
avoiding this "AOSP mode" and using the default (2-space indentation)
mode when formatting a codebase with Google Java Format.

[1] https://github.com/google/google-java-format/wiki/FAQ
[2] https://github.com/psf/black#the-black-code-style
[3] https://github.com/google/google-java-format/issues/19

Liam Newman

unread,
Feb 25, 2021, 4:46:22 AM2/25/21
to Jenkins Developers
Thanks you two, for clearly describing the issue that I'm attempting to address.  
I've updated https://github.com/jenkinsci/github-branch-source-plugin/pull/392 with the following changes: 
Note: I am using the eclipse formatter rather than to google formatter in an attempt to reduce the severity of the diff wall and disruption to folks familiar with the current code style. There are still significant differences, but they are far less than switching to google formatting.  However, that is still an option. You can see what it would look like here: https://github.com/bitwiseman/github-branch-source-plugin/pull/new/task/formatting-google 

Basil Crow

unread,
Feb 25, 2021, 10:27:09 AM2/25/21
to jenkin...@googlegroups.com
On Thu, Feb 25, 2021 at 1:46 AM Liam Newman <bitwi...@gmail.com> wrote:
>
> Thanks you two, for clearly describing the issue that I'm attempting to
> address.

You're welcome! As I mentioned, I've done this a few times and have
learned what it takes to make the changes stick across a large number of
developers.

> Formatting issues will fail the build, source remains unchanged.
> Users may run "mvn spotless:apply" to automatically fix formatting.
> This command is easy and short. When "spotless:check" fails it
> specifically tells users to run "mvn spotless:apply" to fix formatting.

This sounds great! +1 for Spotless. I have been using it for years and
have had nothing but positive experiences with the project and maintainer.

> Note: I am using the eclipse formatter rather than to google formatter

My recommendation is to use the Google Java formatter, but not because I
have any personal preference for the Google style. Rather my reasoning is
as follows. My experience with the Eclipse JDT formatter is that while it
can be made to format the majority of code at high quality, this requires
significant tweaking of the configuration options (as you have seen).
Tweaking opens up the door to endless subjective debates (as you have also
seen):

> If anyone disagrees with that evaluation, feel free to say so and
> explain why.

Having to spend time tweaking formatting configuration options means
increased development time. Having to spend time subjectively debating the
results of those tweaks in a PR increases review time. These outcomes
directly go against the stated goals from my previous message: to reduce
development time and to reduce review time.

While the Google Java formatter implies a more drastic transition, it
generally formats the majority of code at high quality without requiring
tweaking. Indeed, that it and Black are _not_ tweakable eliminates the
endless subjective debates that are typically associated with formatting
changes. In this way, it serves the two goals of reducing development time
and reducing review time.

A transition like this will be disruptive no matter which way you slice
and dice it. Better to bite the bullet and take most of the disruption up
front in order to maximize the long-term gain.

An anecdote: My company has been using the Google Java formatter since
2017, and most complaints stopped after 2018. Since 2019 nobody at my
company even thinks about formatting: the Google Java formatter is Good
Enough™ the majority of the time, it is consistently enforced, and it is
not up for debate. People focus on the logic, not the formatting, just as
intended. If you pick the Eclipse JDT formatter, you'll be focusing on
tweaking formatting configuration and subjectively debating formatting
preferences for years to come.

Basil Crow

unread,
Apr 8, 2023, 8:14:11 PM4/8/23
to jenkin...@googlegroups.com
https://github.com/jenkinsci/plugin-pom/pull/733 is a non-draft PR
that incorporates all previous feedback and reflects practical
experience in the Jenkins and Maven projects.

Basil Crow

unread,
Apr 12, 2023, 1:08:15 AM4/12/23
to jenkin...@googlegroups.com
Thanks to everyone who has adopted the new Spotless configuration. I
am trying to get everyone who was already using Spotless migrated over
to the new configuration. Once that happens, I might even be able to
change the opt-in mechanism from the ugly .mvn_exec_spotless file to a
Maven property.

The next question is: do we want to do the same thing for core and
core components? And if so, do we want it to be opt-in or shall we
apply Spotless unconditionally for core and all core components? I can
do the work, but it would take some time to cover all components. The
question boils down to whether or not the other maintainers are
comfortable with this change.

The last time we did a large formatting change in core (adding
Prettier) the usual concerns were expressed regarding merge conflicts
in open PRs and difficulty of backports. I think we did a pretty good
job of resolving merge conflicts in open PRs, and we backported the
formatting change in order to address concerns from the security team.
We could adopt that (successful) strategy again in this case, assuming
the same concerns still apply.

jn...@cloudbees.com

unread,
Apr 12, 2023, 9:06:02 AM4/12/23
to Jenkins Developers
Please do not troll this out until https://github.com/diffplug/spotless/issues/1559 is fixed - this causes all code to fail for windows users using checkout-as-is and checkin-as-is which is not uncommon for windows users who also share code with the Linux subsytem for windows.

I have battled this for one repository where it was already enabled - and to format the code you need to format everything then apply dos2unix on everything (though should not be windows line ends - and some files may need to be and have appropriate gitattributes).

Rather than helping if the first thing a user sees is a lot of wanrings on attempting to build what is just cloned - it will do the opposite.

and before anyone suggests users should change their local environment that is a non starter. - the projects should not require anything special except git maven and java (at their supported/required versions)

/James

jn...@cloudbees.com

unread,
Apr 12, 2023, 9:06:37 AM4/12/23
to Jenkins Developers
> Please do not troll
that too but I meant roll this out.

Basil Crow

unread,
Apr 12, 2023, 11:09:11 AM4/12/23
to jenkin...@googlegroups.com
On Wed, Apr 12, 2023 at 6:06 AM 'jn...@cloudbees.com' via Jenkins
Developers <jenkin...@googlegroups.com> wrote:
> Please do not troll [sic] this out until https://github.com/diffplug/spotless/issues/1559 is fixed

I am rejecting this request, as Spotless works fine on Windows in the
default configuration. Users of exotic configurations that are not
supported by Spotless on Windows should switch to the default
configuration or contribute support for their desired configuration to
Spotless rather than holding back others.
Reply all
Reply to author
Forward
0 new messages