Conversation
Co-authored-by: simbo1905 <simbo1905@60hertz.com>
Co-authored-by: simbo1905 <simbo1905@60hertz.com>
|
Cursor Agent can help with this pull request. Just |
Summary of ChangesHello @simbo1905, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request clarifies the design philosophy for handling numeric types within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a valuable addition, providing clear documentation and examples for the numeric handling design choices in JsonNumber. The new DESIGN_CHOICES.md file effectively explains the rationale for prioritizing lexical representation and avoiding BigDecimal/BigInteger constructors in the core API. The accompanying examples and tests are well-written and successfully demonstrate the recommended practices. My feedback includes a couple of suggestions to improve the clarity and consistency of the example code, making it even easier for developers to understand.
| LOGGER.info("Executing bigDecimalToJsonNumberRequiresChoosingATextPolicy"); | ||
|
|
||
| var thousand = new BigDecimal("1000"); | ||
|
|
||
| var plain = JsonNumber.of(thousand.toPlainString()); | ||
| assertThat(plain.toString()).isEqualTo("1000"); | ||
|
|
||
| var scientific = JsonNumber.of(new BigDecimal("1E+3").toString()); | ||
| assertThat(scientific.toString()).isEqualTo("1E+3"); |
There was a problem hiding this comment.
The logic in this test can be made clearer and more consistent. The first part of the test defines a BigDecimal variable thousand, but the second part creates a BigDecimal inline. For better readability and to more clearly demonstrate the policies, consider defining separate, descriptively named variables for both cases. This makes the intent of each part of the test more explicit.
LOGGER.info("Executing bigDecimalToJsonNumberRequiresChoosingATextPolicy");
// Using toPlainString() for a plain number representation
var bdPlain = new BigDecimal("1000");
var plain = JsonNumber.of(bdPlain.toPlainString());
assertThat(plain.toString()).isEqualTo("1000");
// Using toString() which may produce scientific notation
var bdScientific = new BigDecimal("1E+3");
var scientific = JsonNumber.of(bdScientific.toString());
assertThat(scientific.toString()).isEqualTo("1E+3");| public static JsonNumber bigDecimalToJsonNumberChooseTextPolicy() { | ||
| var bd = new BigDecimal("1000"); | ||
|
|
||
| var plain = JsonNumber.of(bd.toPlainString()); | ||
| System.out.println("BigDecimal.toPlainString() -> JsonNumber: " + plain); | ||
|
|
||
| var scientific = JsonNumber.of(new BigDecimal("1E+3").toString()); | ||
| System.out.println("BigDecimal.toString() -> JsonNumber: " + scientific); | ||
|
|
||
| return plain; | ||
| } |
There was a problem hiding this comment.
This example method can be made clearer and more consistent. The first part defines a BigDecimal variable bd, but the second part creates a BigDecimal inline. To improve readability and better illustrate the different text policies, it would be beneficial to use separate, descriptively named variables for both the "plain" and "scientific" cases.
public static JsonNumber bigDecimalToJsonNumberChooseTextPolicy() {
// Example with toPlainString() to avoid scientific notation.
var bdPlain = new BigDecimal("1000");
var plain = JsonNumber.of(bdPlain.toPlainString());
System.out.println("BigDecimal.toPlainString() -> JsonNumber: " + plain);
// Example with toString(), which may use scientific notation.
var bdScientific = new BigDecimal("1E+3");
var scientific = JsonNumber.of(bdScientific.toString());
System.out.println("BigDecimal.toString() -> JsonNumber: " + scientific);
return plain;
}Co-authored-by: simbo1905 <simbo1905@60hertz.com>
Co-authored-by: simbo1905 <simbo1905@60hertz.com>
Co-authored-by: simbo1905 <simbo1905@60hertz.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0903da3ef8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case JsonArray a -> a.elements().stream().map(DesignChoicesExamples::toNative).toList(); | ||
| case JsonObject o -> o.members().entrySet().stream() | ||
| .collect(Collectors.toMap(Map.Entry::getKey, e -> toNative(e.getValue()))); |
There was a problem hiding this comment.
Handle JsonNull values when collecting object members
toNative returns null for JsonNull, but the Collectors.toMap(...) call rejects null values and will throw a NullPointerException when a JSON object contains a null member value. This makes the documented “Map/List/primitives” recipe fail on valid JSON like {"a": null}. Consider collecting into a map that tolerates null values (e.g., manual HashMap population) or mapping JsonNull to a sentinel instead of null.
Useful? React with 👍 / 👎.
Co-authored-by: simbo1905 <simbo1905@60hertz.com>
Co-authored-by: simbo1905 <simbo1905@60hertz.com>
What changed
DESIGN_CHOICES.mdto explain the rationale behindJsonNumber's design, particularly regardingBigDecimal/BigIntegerconstructors.README.mdto link to the new design document.DesignChoicesExamples.java) and corresponding JUnit tests (DesignChoicesNumberExamplesTest.java) demonstrating the recommended approach for numeric handling.Why this change is needed
This change clarifies why
BigDecimal/BigIntegerconstructors were removed fromJsonNumberduring an upstream sync. The upstream design prioritizes:BigDecimal/BigIntegerfactories would force formatting policies (e.g.,toString()vstoPlainString()) into the core API.The new documentation and examples provide clear guidance on how to perform lossless numeric interoperability using
JsonNumber.toString()andBigDecimal/BigIntegerexplicitly.How were these changes tested
DesignChoicesNumberExamplesTest.java) were added to verify the examples and design principles.$(command -v mvnd || command -v mvn || command -v ./mvnw) -pl json-java21 test -Djava.util.logging.ConsoleHandler.level=INFO, which reportedBUILD SUCCESS.Checklist
CODING_STYLE_LLM.mdconvensionsAGENTS.mdupdated if appropriate