Provide common Java version detection #921

fvgh · 2021-08-25T19:15:41Z

The approach of GoogleJavaFormatter to provide default versions depending of the JRE Java version has been extracted to a generic class. The code duplicated in testlib JRE has been removed.

Furthermore the approach has been extended:

It allows to check whether a formatter version is supported by the current JRE. This approach allows to provide proposals about other formatter versions to the user.
It provides version proposals not requiring a JRE upgrade if possible.

For these extensions the amount of code has been increased. It needs to be discussed whether the benefit is worth the increase of code complexity.

fvgh · 2021-08-25T19:34:43Z

There is still work in progress. Tests are missing. Comments on scope and readability of the new Jvm class are more than welcome.

nedtwigg

I think this is an awesome idea. I have one general concern which I outlined below. I suppose we could watch for new releases of all the formatters which we support and be more proactive about bumping our defaults. I don't think we maintainers should have to do all of that, but we could generate error messages which encourage people to generate PRs, it's very easy for us to merge and cut releases for them.

But I tend to think that it's too noisy for us to release a new Spotless any time any one of our formatters releases a new version. If the average project releases N times per year, and we support M formatters, then we are going to have to release N*M times per year, and that creates noise for anyone who is trying to keep their Spotless up to date.

lib/src/main/java/com/diffplug/spotless/java/GoogleJavaFormatStep.java

lib/src/main/java/com/diffplug/spotless/Jvm.java

fvgh · 2021-08-28T12:31:09Z

release a new Spotless any time any one of our formatters releases a new version

I think it is beneficial for the users when default versions are increased frequently, since the user only needs to increase the Spotless version. But I think that everybody who automated the formatting with Spotless can wait for a few month for a new release. So just because someone updated a formatter version, I don't see the need that a new Spotless release follows a few days later.

This JVM check is just another assistance utility, since many projects still stuck with Java 8.

@nedtwigg
Not sure about your general remarks. Do you mean that instead of having a few utility functions, we should aim for something different?

nedtwigg · 2021-08-30T18:51:59Z

After your most recent changes this whole thing looks great to me.

Ironed out corner cases. Added Support#verifyConfiguration. Slight modifications in wording and method names (see GoogleJavaFormatStep / GoogleJavaFormatStepTest).

testlib/src/test/java/com/diffplug/spotless/JvmTest.java

fvgh · 2021-09-12T12:06:28Z

lib/src/main/java/com/diffplug/spotless/Jvm.java

+		public void verifyConfiguration() {
+			if (jvm2fmtVersion.size() > fmt2jvmVersion.size()) {
+				throw new AssertionError("The added support information contains duplicated formatter versions.");
+			}
+			if (jvm2fmtVersion.size() < fmt2jvmVersion.size()) {
+				throw new AssertionError("The added support information contains duplicated JVM versions.");
+			}
+			jvm2fmtVersion.forEach((jvmVersion, fmtVersion) -> {
+				Map.Entry<Integer, V> lower = jvm2fmtVersion.lowerEntry(jvmVersion);
+				if ((null != lower) && (fmtVersionComparator.compare(fmtVersion, lower.getValue()) <= 0)) {
+					throw new AssertionError(String.format("%d/%s should be lower than %d/%s", jvmVersion, fmtVersion, lower.getKey(), lower.getValue()));
+				}
+				Map.Entry<Integer, V> higher = jvm2fmtVersion.higherEntry(jvmVersion);
+				if ((null != higher) && (fmtVersionComparator.compare(fmtVersion, higher.getValue()) >= 0)) {
+					throw new AssertionError(String.format("%d/%s should be higher than %d/%s", jvmVersion, fmtVersion, higher.getKey(), higher.getValue()));
+				}
+			});
+		}


@nedtwigg
I moved unnecessary checks to an external method. My problem with Jvm.Support is the static configuration. It fits best with the current formatter step implementations, but it is not lazy. So I prefer to do sanity checks in the UTs.
What do you think?

Darn, my comment showed up twice, so I deleted one and they both got deleted :(

I think the sanity checks are fast enough that they cost ~0 to users, but adding a chore to the unit tests costs 10s of minutes per PR, so I lean very lightly towards making the verification automatic rather than adding them to a unit test checklist. I'm fine either way though, the core idea here is good.

# Conflicts: # lib/src/main/java/com/diffplug/spotless/java/GoogleJavaFormatStep.java # testlib/src/test/java/com/diffplug/spotless/java/GoogleJavaFormatStepTest.java

fvgh · 2021-09-12T13:51:38Z

@nedtwigg Propose to do a "squash and merge". Didn't want to do a rebase and merged main instead to keep track of the conflicts.

nedtwigg

This is looking great to me :)

nedtwigg · 2021-09-14T15:22:33Z

Btw, squash and merge sounds good, I'll let you push the button when you're ready. If there's more you want to put in this one then take your time :)

fvgh · 2021-09-14T20:10:16Z

@nedtwigg Tripped again over the circle-ci test_nomaven... problem, but saw that com.diffplug.spotless:spotless-eclipse-groovy:4.2.0 was published.

However, I am afraid we made a mistake when closing #877.

I never published a com.diffplug.spotless:spotless-eclipse-groovy:4.1.1, nor did I do an update afterwards of the corresponding dependency locks in lib-extra. Hence we never provided a fixed version. Think we owe @k-brooks a big apology.

I will provide all formatter updates for Eclipse 4.20 at this weekend. But they will all require JVM 11+.

@k-brooks : Sorry, about the inconvenience. Is Eclipse 4.20 / JVM 11+ fine for you, or do you need JVM 8 support?

Provide common Java version detection

ae70362

fvgh force-pushed the common_jvm_detection branch from 80ba06d to ae70362 Compare August 25, 2021 19:20

fvgh requested a review from a team August 25, 2021 19:32

fvgh marked this pull request as draft August 25, 2021 19:35

nedtwigg reviewed Aug 26, 2021

View reviewed changes

lib/src/main/java/com/diffplug/spotless/java/GoogleJavaFormatStep.java Show resolved Hide resolved

lib/src/main/java/com/diffplug/spotless/Jvm.java Outdated Show resolved Hide resolved

Improoved some range limit handling. Finalized when implementing tests.

7879193

Provided tests for new functionality.

174e57f

Ironed out corner cases. Added Support#verifyConfiguration. Slight modifications in wording and method names (see GoogleJavaFormatStep / GoogleJavaFormatStepTest).

fvgh commented Sep 12, 2021

View reviewed changes

testlib/src/test/java/com/diffplug/spotless/JvmTest.java Outdated Show resolved Hide resolved

fvgh commented Sep 12, 2021

View reviewed changes

fvgh added 2 commits September 12, 2021 14:36

Merge remote-tracking branch 'origin/main' into common_jvm_detection

10716d1

# Conflicts: # lib/src/main/java/com/diffplug/spotless/java/GoogleJavaFormatStep.java # testlib/src/test/java/com/diffplug/spotless/java/GoogleJavaFormatStepTest.java

Proposal for test clean-up.

30336b1

fvgh force-pushed the common_jvm_detection branch from 6824987 to 30336b1 Compare September 12, 2021 13:25

fvgh marked this pull request as ready for review September 12, 2021 13:47

nedtwigg approved these changes Sep 13, 2021

View reviewed changes

Verify support directly in add method

3be218b

fvgh merged commit 7b90966 into main Sep 13, 2021

fvgh deleted the common_jvm_detection branch September 13, 2021 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide common Java version detection #921

Provide common Java version detection #921

fvgh commented Aug 25, 2021 •

edited

fvgh commented Aug 25, 2021

nedtwigg left a comment •

edited

fvgh commented Aug 28, 2021 •

edited

nedtwigg commented Aug 30, 2021 •

edited

fvgh Sep 12, 2021 •

edited

nedtwigg Sep 13, 2021

fvgh commented Sep 12, 2021

nedtwigg left a comment

nedtwigg commented Sep 14, 2021

fvgh commented Sep 14, 2021

Provide common Java version detection #921

Provide common Java version detection #921

Conversation

fvgh commented Aug 25, 2021 • edited

fvgh commented Aug 25, 2021

nedtwigg left a comment • edited

Choose a reason for hiding this comment

fvgh commented Aug 28, 2021 • edited

nedtwigg commented Aug 30, 2021 • edited

fvgh Sep 12, 2021 • edited

Choose a reason for hiding this comment

nedtwigg Sep 13, 2021

Choose a reason for hiding this comment

fvgh commented Sep 12, 2021

nedtwigg left a comment

Choose a reason for hiding this comment

nedtwigg commented Sep 14, 2021

fvgh commented Sep 14, 2021

fvgh commented Aug 25, 2021 •

edited

nedtwigg left a comment •

edited

fvgh commented Aug 28, 2021 •

edited

nedtwigg commented Aug 30, 2021 •

edited

fvgh Sep 12, 2021 •

edited