Perform minor code clean-up #2544

Marcono1234 · 2023-11-18T20:34:03Z

Purpose

Performs some minor code clean-up, which should not noticeably (or at all) change functionality

Description

Notable changes:

Replace most usages of <code> with {@code ...} in Javadoc
Add proper summary sentence to GsonBuilder.enableComplexMapKeySerialization
Extend documentation for GsonBuilder.setDateFormat methods
Fix TypeToken.isAssignableFrom(Type) throwing AssertionError in some cases
Maybe the method should not throw an exception in the first place, but it might not matter much since it is deprecated already anyway.
Remove outdated throws NumberFormatException from internal JsonReader.nextQuotedValue; the behavior had been changed by 85ebaf7
Fix incorrect documentation on JsonScope fields
Fix unit tests having 'expected' and 'actual' switched
For that I removed the .getType() call from TypeToken to use <T> Gson.fromJson(..., TypeToken<T>) instead of <T> Gson.fromJson(..., Type) because otherwise the call to assertThat(...) would have been ambiguous.
Use dedicated Truth methods instead of isTrue() / isFalse()
Use common helper methods in JsonWriterTest to avoid duplication
Implement toString() for ReflectionAccessFilter constants

Checklist

New code follows the Google Java Style Guide
This is automatically checked by mvn verify, but can also be checked on its own using mvn spotless:check.
Style violations can be fixed using mvn spotless:apply; this can be done in a separate commit to verify that it did not cause undesired changes.
If necessary, new public API validates arguments, for example rejects null
New public API has Javadoc
- Javadoc uses @since $next-version$
  ( $next-version$ is a special placeholder which is automatically replaced during release)
If necessary, new unit tests have been added
- Assertions in unit tests use Truth, see existing tests
- No JUnit 3 features are used (such as extending class TestCase)
- If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
mvn clean verify javadoc:jar passes without errors

Notable changes: - Replace most usages of `<code>` with `{@code ...}` in Javadoc - Add proper summary sentence to `GsonBuilder.enableComplexMapKeySerialization` - Extend documentation for `GsonBuilder.setDateFormat` methods - Fix `TypeToken.isAssignableFrom(Type)` throwing AssertionError in some cases Maybe the method should not throw an exception in the first place, but it might not matter much since it is deprecated already anyway. - Remove outdated `throws NumberFormatException` from internal `JsonReader.nextQuotedValue`; the behavior had been changed by 85ebaf7 - Fix incorrect documentation on JsonScope fields - Fix unit tests having 'expected' and 'actual' switched - Use dedicated Truth methods instead of `isTrue()` / `isFalse()` - Use common helper methods in JsonWriterTest to avoid duplication

gson/src/test/java/com/google/gson/JsonPrimitiveTest.java

gson/src/test/java/com/google/gson/functional/ObjectTest.java

eamonnmcmanus

Very nice! Just a few comments.

...droid-proguard-example/src/com/google/gson/examples/android/GsonProguardExampleActivity.java

gson/src/main/java/com/google/gson/reflect/TypeToken.java

gson/src/main/java/com/google/gson/stream/JsonReader.java

gson/src/test/java/com/google/gson/MockExclusionStrategy.java

gson/src/test/java/com/google/gson/functional/MapTest.java

gson/src/test/java/com/google/gson/functional/ObjectTest.java

Actually it looks like the warning is not actually caused by usage of `HEAD` as value, but rather because the project has a snapshot version during development (which is expected though), see https://github.com/apache/maven-artifact-plugin/blob/maven-artifact-plugin-3.5.0/src/main/java/org/apache/maven/plugins/artifact/buildinfo/BuildInfoWriter.java#L140 But this is not a problem either since during release a non-snapshot version is used.

eamonnmcmanus

Looking good! Just one small thing...

eamonnmcmanus · 2023-11-19T16:42:51Z

gson/src/main/java/com/google/gson/ReflectionAccessFilter.java

@@ -117,6 +117,11 @@ public FilterResult check(Class<?> rawClass) {
              ? FilterResult.BLOCK_INACCESSIBLE
              : FilterResult.INDECISIVE;
        }
+
+        @Override


Can you mention these in the PR description?

Have included it now; sorry that I added this in a follow-up commit and not in the original changes of this PR. I just remembered that this change was something I wanted to do for quite some time because the default toString() during debugging makes it difficult to understand which filter is used.

eamonnmcmanus

Thanks again. This is a big improvement!

* Perform minor code clean-up Notable changes: - Replace most usages of `<code>` with `{@code ...}` in Javadoc - Add proper summary sentence to `GsonBuilder.enableComplexMapKeySerialization` - Extend documentation for `GsonBuilder.setDateFormat` methods - Fix `TypeToken.isAssignableFrom(Type)` throwing AssertionError in some cases Maybe the method should not throw an exception in the first place, but it might not matter much since it is deprecated already anyway. - Remove outdated `throws NumberFormatException` from internal `JsonReader.nextQuotedValue`; the behavior had been changed by 85ebaf7 - Fix incorrect documentation on JsonScope fields - Fix unit tests having 'expected' and 'actual' switched - Use dedicated Truth methods instead of `isTrue()` / `isFalse()` - Use common helper methods in JsonWriterTest to avoid duplication * Implement `toString()` for ReflectionAccessFilter constants * Address most of the review comments * Add comment about `source.scm.tag=HEAD` warning Actually it looks like the warning is not actually caused by usage of `HEAD` as value, but rather because the project has a snapshot version during development (which is expected though), see https://github.com/apache/maven-artifact-plugin/blob/maven-artifact-plugin-3.5.0/src/main/java/org/apache/maven/plugins/artifact/buildinfo/BuildInfoWriter.java#L140 But this is not a problem either since during release a non-snapshot version is used.

Marcono1234 commented Nov 18, 2023

View reviewed changes

gson/src/test/java/com/google/gson/JsonPrimitiveTest.java Show resolved Hide resolved

gson/src/test/java/com/google/gson/functional/ObjectTest.java Show resolved Hide resolved

eamonnmcmanus reviewed Nov 18, 2023

View reviewed changes

Marcono1234 added 4 commits November 19, 2023 14:27

Implement toString() for ReflectionAccessFilter constants

76c83bb

Address most of the review comments

48e664c

Merge branch 'main' into marcono1234/minor-code-cleanup

79d04c8

Marcono1234 force-pushed the marcono1234/minor-code-cleanup branch from 6197235 to f906b3a Compare November 19, 2023 14:41

eamonnmcmanus requested changes Nov 19, 2023

View reviewed changes

eamonnmcmanus approved these changes Nov 19, 2023

View reviewed changes

eamonnmcmanus merged commit 08174e2 into google:main Nov 19, 2023
10 checks passed

Marcono1234 deleted the marcono1234/minor-code-cleanup branch November 19, 2023 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform minor code clean-up #2544

Perform minor code clean-up #2544

Marcono1234 commented Nov 18, 2023 •

edited

Loading

eamonnmcmanus left a comment

eamonnmcmanus left a comment

eamonnmcmanus Nov 19, 2023

Marcono1234 Nov 19, 2023

eamonnmcmanus left a comment

Perform minor code clean-up #2544

Perform minor code clean-up #2544

Conversation

Marcono1234 commented Nov 18, 2023 • edited Loading

Purpose

Description

Checklist

eamonnmcmanus left a comment

Choose a reason for hiding this comment

eamonnmcmanus left a comment

Choose a reason for hiding this comment

eamonnmcmanus Nov 19, 2023

Choose a reason for hiding this comment

Marcono1234 Nov 19, 2023

Choose a reason for hiding this comment

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Marcono1234 commented Nov 18, 2023 •

edited

Loading