#438 Upgrade to support jdk17 #477

hbzhou · 2024-09-23T03:12:25Z

try to build with JDK17 since it's LTS version and widely used in organizations

codescene-delta-analysis

✅ Code Health Quality Gates: OK

Change in average Code Health of affected files: -0.00 (9.77 -> 9.77)

View detailed results in CodeScene

Absence of Expected Change Pattern

jsonschema-generator/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/MethodScope.java is usually changed with: jsonschema-generator/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/FieldScope.java
jsonschema-generator/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/SchemaGeneratorConfigImpl.java is usually changed with: jsonschema-generator/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/SchemaGeneratorConfig.java

codescene-delta-analysis

✅ Code Health Quality Gates: OK

Change in average Code Health of affected files: -0.00 (9.77 -> 9.77)

View detailed results in CodeScene

Absence of Expected Change Pattern

jsonschema-generator/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/MethodScope.java is usually changed with: jsonschema-generator/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/FieldScope.java
jsonschema-generator/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/SchemaGeneratorConfigImpl.java is usually changed with: jsonschema-generator/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/SchemaGeneratorConfig.java

codescene-delta-analysis

✅ Code Health Quality Gates: OK

Change in average Code Health of affected files: -0.00 (9.77 -> 9.77)

View detailed results in CodeScene

Absence of Expected Change Pattern

jsonschema-generator/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/MethodScope.java is usually changed with: jsonschema-generator/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/FieldScope.java
jsonschema-generator/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/SchemaGeneratorConfigImpl.java is usually changed with: jsonschema-generator/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/SchemaGeneratorConfig.java

CarstenWickner

I appreciate all the work that went into this, I'm just not sure how to best approach abandoning JDK 11 support.
The library could be used in bigger applications, that might still be tied to JDK 11.

I'm pondering a separate branch to keep supporting both JDK versions or simply making this a new major version, that expects JDK 17+, since there aren't many changes being added anyway, that an application still on JDK 11 couldn't live without.

That's why I haven't acted upon your PR yet.
I'm undecided how to tackle it.

CarstenWickner · 2024-10-14T20:06:29Z

.github/workflows/codeql-analysis.yml

+    - name: Setup Java 17
+      uses: actions/setup-java@v1
+      with:
+        java-version: 17


question: Why is this needed for CodeQL now?

CarstenWickner · 2024-10-14T20:06:55Z

.github/workflows/codeql-analysis.yml

    - name: Autobuild
      uses: github/codeql-action/autobuild@v2

+


nitpick: one empty line is enough 😛

Suggested change

CarstenWickner · 2024-10-14T20:07:58Z

jsonschema-generator-bom/pom.xml

@@ -12,6 +12,7 @@
    <packaging>pom</packaging>

    <properties>
+    <maven.compiler.release>17</maven.compiler.release>


suggestion: Irrelevant compiler setting for a BOM.

Suggested change

<maven.compiler.release>17</maven.compiler.release>

CarstenWickner · 2024-10-14T20:09:31Z

jsonschema-generator-parent/pom.xml

+        <maven.compiler.source>17</maven.compiler.source>
+        <maven.compiler.target>17</maven.compiler.target>


suggestion: Changing these settings and then not using them feels somewhat pointless.
Fair enough to switch to the release, but then let's please introduce a property for that again to be consistent.

Suggested change

<maven.compiler.source>17</maven.compiler.source>

<maven.compiler.target>17</maven.compiler.target>

<maven.compiler.release>17</maven.compiler.release>

CarstenWickner · 2024-10-14T20:10:19Z

jsonschema-generator-parent/pom.xml

                        <showDeprecation>true</showDeprecation>
+            <release>17</release>


suggestion: Let's please continue using the respective property here.

Suggested change

<release>17</release>

<release>${maven.compiler.release}</release>

CarstenWickner · 2024-10-14T20:12:51Z

...enerator/src/main/java/com/github/victools/jsonschema/generator/impl/AttributeCollector.java

+        } else if (value instanceof Boolean boolean1) {
+            node.add(boolean1);
+        } else if (value instanceof Double double1) {
+            node.add(double1);
+        } else if (value instanceof Float float1) {
+            node.add(float1);


suggestion: Let's please avoid ambiguous naming wherever possible, including not assigning numbered variable name, even for such short-lived ones.

Suggested change

} else if (value instanceof Boolean boolean1) {

node.add(boolean1);

} else if (value instanceof Double double1) {

node.add(double1);

} else if (value instanceof Float float1) {

node.add(float1);

} else if (value instanceof Boolean booleanValue) {

node.add(booleanValue);

} else if (value instanceof Double doubleValue) {

node.add(doubleValue);

} else if (value instanceof Float floatValue) {

node.add(floatValue);

CarstenWickner · 2024-10-14T20:15:09Z

...src/main/java/com/github/victools/jsonschema/generator/impl/SchemaGenerationContextImpl.java

+        if (targetScope instanceof FieldScope scope && !scope.isFakeContainerItemScope()) {
+            return this.populateFieldSchema(scope.asFakeContainerItemScope());


nitpick: A less ambiguous variable would be nice.

Suggested change

if (targetScope instanceof FieldScope scope && !scope.isFakeContainerItemScope()) {

return this.populateFieldSchema(scope.asFakeContainerItemScope());

if (targetScope instanceof FieldScope fieldScope && !fieldScope.isFakeContainerItemScope()) {

return this.populateFieldSchema(fieldScope.asFakeContainerItemScope());

CarstenWickner · 2024-10-14T20:15:40Z

...src/main/java/com/github/victools/jsonschema/generator/impl/SchemaGenerationContextImpl.java

+        if (targetScope instanceof MethodScope scope && !scope.isFakeContainerItemScope()) {
+            return this.populateMethodSchema(scope.asFakeContainerItemScope());


nitpick: Similarly here.

Suggested change

if (targetScope instanceof MethodScope scope && !scope.isFakeContainerItemScope()) {

return this.populateMethodSchema(scope.asFakeContainerItemScope());

if (targetScope instanceof MethodScope methodScope && !methodScope.isFakeContainerItemScope()) {

return this.populateMethodSchema(methodScope.asFakeContainerItemScope());

CarstenWickner · 2024-10-14T20:18:19Z

pom.xml

+  <properties>
+    <maven.compiler.release>17</maven.compiler.release>
+  </properties>


suggestion: Irrrelevant compiler setting in a reactor POM.

Suggested change

<properties>

<maven.compiler.release>17</maven.compiler.release>

</properties>

Jeremy Zhou added 4 commits September 20, 2024 17:53

upgrade to jdk17 with openwrite

c486b94

setup java 17

b2eced4

setup java 17

632bb9e

update maven version to fit jdk17

f89bb4e

codescene-delta-analysis bot approved these changes Sep 23, 2024

View reviewed changes

hbzhou changed the title ~~Upgrade jdk17~~ #438 Upgrade jdk17 Sep 23, 2024

hbzhou marked this pull request as draft September 23, 2024 03:18

hbzhou closed this Sep 24, 2024

hbzhou reopened this Sep 24, 2024

hbzhou marked this pull request as ready for review September 24, 2024 06:55

codescene-delta-analysis bot approved these changes Sep 24, 2024

View reviewed changes

hbzhou changed the title ~~#438 Upgrade jdk17~~ #438 Upgrade to support jdk17 Sep 24, 2024

Merge branch 'main' into upgrade_jdk17

1633c3d

codescene-delta-analysis bot approved these changes Oct 9, 2024

View reviewed changes

CarstenWickner reviewed Oct 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#438 Upgrade to support jdk17 #477

#438 Upgrade to support jdk17 #477

hbzhou commented Sep 23, 2024

codescene-delta-analysis bot left a comment

codescene-delta-analysis bot left a comment

codescene-delta-analysis bot left a comment

CarstenWickner left a comment

CarstenWickner Oct 14, 2024

CarstenWickner Oct 14, 2024

CarstenWickner Oct 14, 2024

CarstenWickner Oct 14, 2024

CarstenWickner Oct 14, 2024

CarstenWickner Oct 14, 2024

CarstenWickner Oct 14, 2024

CarstenWickner Oct 14, 2024

CarstenWickner Oct 14, 2024

		<maven.compiler.source>17</maven.compiler.source>
		<maven.compiler.target>17</maven.compiler.target>

	<maven.compiler.source>17</maven.compiler.source>
	<maven.compiler.target>17</maven.compiler.target>
	<maven.compiler.release>17</maven.compiler.release>

		<showDeprecation>true</showDeprecation>
		<release>17</release>

	<release>17</release>
	<release>${maven.compiler.release}</release>

		if (targetScope instanceof FieldScope scope && !scope.isFakeContainerItemScope()) {
		return this.populateFieldSchema(scope.asFakeContainerItemScope());

		if (targetScope instanceof MethodScope scope && !scope.isFakeContainerItemScope()) {
		return this.populateMethodSchema(scope.asFakeContainerItemScope());

	<properties>
	<maven.compiler.release>17</maven.compiler.release>
	</properties>

#438 Upgrade to support jdk17 #477

Are you sure you want to change the base?

#438 Upgrade to support jdk17 #477

Conversation

hbzhou commented Sep 23, 2024

codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

Absence of Expected Change Pattern

codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

Absence of Expected Change Pattern

codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

Absence of Expected Change Pattern

CarstenWickner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment