Use JsonUtil.getJsonX more often #10071
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This looks like it should have been final
And add javadoc to the methods
pdurbin
added
Size: 3
sekmiller
approved these changes
Jul 23, 2024
stevenwinship
added a commit
that referenced
this pull request
Aug 2, 2024
commit 0d27957 Author: Ben Companjen <ben@companjen.name> Date: Wed Jul 31 20:55:11 2024 +0200 Use JsonUtil.getJsonX more often (#10071) * Update MetricsUtilTest classifiers * Add private MetricsUtil constructor * Make public static field also final This looks like it should have been final * Use Java's standard order of field classifiers * Use empty diamond with generic constructors * Return value directly * Put long notes at top of javadoc * Call JsonUtil.getJsonX to prevent resource leak And add javadoc to the methods * Test StringToJsonX returns null on null
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
When I found the static methods
stringToJsonObjectandstringToJsonArrayinMetricsUtilwhile working onJsonUtil.getJsonObjectandgetJsonArray(in #10062), I saw an opportunity for deduplicating efforts. It looked like theMetricsUtilmethods were not tested either (in Coveralls) and would not close theJsonReaderproperly if something goes wrong (no try-with-resources ortry-finally). This PR addresses these issues and fixes some minor other things in the same files.MetricsUtilTest, but because they were in a static inner class, they did not run. I removed thestaticmodifier and now they work again.JsonUtilfrom within theMetricsUtilmethods so that the method signature doesn't change while resources close on exceptionsstr == nullpathMetricsUtiland remove constructing it from the test class, because it is a utility class that should not be instantiatedpublic staticfieldYEAR_AND_MONTH_PATTERNwas notfinal=publicmodifiers from tests and test classesWhich issue(s) this PR closes:
Closes #10061
Special notes for your reviewer:
Please check the new test. This work is related to #10062, although this is mostly syntactic cleanup.
Sonarlint still recommends to have
stringToJsonObjectandstringToJsonArrayreturn an empty collection instead ofnull, but that is a bigger refactor.Suggestions on how to test this:
See that the new and re-enabled tests pass.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No
Is there a release notes update needed for this change?:
No
Additional documentation:
n/a