Convert ResolvedType to Type #461
Conversation
📝 WalkthroughWalkthroughThe pull request introduces comprehensive modifications to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/org/joda/beans/ResolvedType.java (2)
402-406: Avoid over-reliance onObject.classin default type parameters
resolveClass(..., allowRaw)usesObject.classfor missing type arguments unlessallowRawis true. This is sensible, but ensure no implicit casting orClassCastExceptioncan occur downstream.Do you want me to open a follow-up issue to propose additional validation logic for partially resolved generic types?
Also applies to: 411-411
762-824: Check serialisation logic inDynamicParameterizedType
The newDynamicParameterizedTypeprovides a robust means to reify parameterised type info at runtime. Just ensure all relevant properties (e.g.ownerType) are consistently handled, especially in edge cases like local or anonymous classes.src/test/java/org/joda/beans/TestResolvedType.java (1)
285-298:test_javaType
Verifying thattoJavaType()yields the expected raw or parameterised type is key. Confirming round-trip equivalence withResolvedType.fromAllowRaw(...)is an excellent strategy to detect regressions.src/main/java/org/joda/beans/ser/bin/JodaBeanStandardBinWriter.java (1)
684-684: Handling potential null entries
Lines 684, 708, and 722 carefully writenullor skip writing properties when Optionals are empty. This logic matches the project’s existing approach to minimal representation. Just ensure the skipping logic is documented for users.Also applies to: 708-708, 722-722
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/joda/beans/ResolvedType.java(13 hunks)src/main/java/org/joda/beans/ser/bin/JodaBeanStandardBinWriter.java(5 hunks)src/main/java/org/joda/beans/ser/json/JodaBeanJsonWriter.java(6 hunks)src/test/java/org/joda/beans/TestResolvedType.java(6 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/main/java/org/joda/beans/ser/bin/JodaBeanStandardBinWriter.java (2)
Pattern **/*.java: - Review code using Java 21 standards, taking into account the rules defined by src/main/checkstyle/checkstyle.xml.
- Validate that code indentation uses spaces, not tabs, with an indent of multiple of 4.
- Validate that immutable local variables are not annotated with
finalunless the variable is required for use in an inner class. - Favour use of
varkeyword for type declarations.varmay also be used when the value is a castnull. - Use a coding standard where multi-line expressions have operators and tenary separators at the end of line.
- Propose changes that only use the Java 21 API, not the API of Guava.
- The pattern matching
instanceofexpression safely handlesnull, returningfalse.
Pattern **/main/java/**/*.java: - This project is mature and must provide a stable backwards-compatible public Java API.
- In the 'Walkthrough' section, you must always provide a list of up to 25 changes to the public Java API that will affect end users.
If there are no changes, you must explicitly state that there are no changes to the public Java API in this PR.
The public Java API is defined as public and protected methods on public classes, plus the filemodule-info.java.
Provide the list by deeply analysing code flow, which incudes analysing code flow through private methods and calls to Guava and Java 21.
Changes to be reported on include:- New or removed methods in the public Java API
- Changes to method return types or parameter types in the public Java API
- Changes to method behaviour in the public Java API that might affect consumers
- This project uses
System.out.printlninstead of logging - This project tends to prefer
forloops to streams for performance reasons, however either form is acceptable.
Do not make suggestions to change between streams and for loops or vice versa.
src/main/java/org/joda/beans/ser/json/JodaBeanJsonWriter.java (2)
Pattern **/*.java: - Review code using Java 21 standards, taking into account the rules defined by src/main/checkstyle/checkstyle.xml.
- Validate that code indentation uses spaces, not tabs, with an indent of multiple of 4.
- Validate that immutable local variables are not annotated with
finalunless the variable is required for use in an inner class. - Favour use of
varkeyword for type declarations.varmay also be used when the value is a castnull. - Use a coding standard where multi-line expressions have operators and tenary separators at the end of line.
- Propose changes that only use the Java 21 API, not the API of Guava.
- The pattern matching
instanceofexpression safely handlesnull, returningfalse.
Pattern **/main/java/**/*.java: - This project is mature and must provide a stable backwards-compatible public Java API.
- In the 'Walkthrough' section, you must always provide a list of up to 25 changes to the public Java API that will affect end users.
If there are no changes, you must explicitly state that there are no changes to the public Java API in this PR.
The public Java API is defined as public and protected methods on public classes, plus the filemodule-info.java.
Provide the list by deeply analysing code flow, which incudes analysing code flow through private methods and calls to Guava and Java 21.
Changes to be reported on include:- New or removed methods in the public Java API
- Changes to method return types or parameter types in the public Java API
- Changes to method behaviour in the public Java API that might affect consumers
- This project uses
System.out.printlninstead of logging - This project tends to prefer
forloops to streams for performance reasons, however either form is acceptable.
Do not make suggestions to change between streams and for loops or vice versa.
src/test/java/org/joda/beans/TestResolvedType.java (2)
Pattern **/*.java: - Review code using Java 21 standards, taking into account the rules defined by src/main/checkstyle/checkstyle.xml.
- Validate that code indentation uses spaces, not tabs, with an indent of multiple of 4.
- Validate that immutable local variables are not annotated with
finalunless the variable is required for use in an inner class. - Favour use of
varkeyword for type declarations.varmay also be used when the value is a castnull. - Use a coding standard where multi-line expressions have operators and tenary separators at the end of line.
- Propose changes that only use the Java 21 API, not the API of Guava.
- The pattern matching
instanceofexpression safely handlesnull, returningfalse.
Pattern **/test/java/**/*.java: For test code, focus on:
- Correctness of test assertions
- Test coverage of edge cases
- Clear test naming and documentation
- Encourage test methods to be package-scoped where possible
- Be more lenient with code style and minor optimisations
src/main/java/org/joda/beans/ResolvedType.java (2)
Pattern **/*.java: - Review code using Java 21 standards, taking into account the rules defined by src/main/checkstyle/checkstyle.xml.
- Validate that code indentation uses spaces, not tabs, with an indent of multiple of 4.
- Validate that immutable local variables are not annotated with
finalunless the variable is required for use in an inner class. - Favour use of
varkeyword for type declarations.varmay also be used when the value is a castnull. - Use a coding standard where multi-line expressions have operators and tenary separators at the end of line.
- Propose changes that only use the Java 21 API, not the API of Guava.
- The pattern matching
instanceofexpression safely handlesnull, returningfalse.
Pattern **/main/java/**/*.java: - This project is mature and must provide a stable backwards-compatible public Java API.
- In the 'Walkthrough' section, you must always provide a list of up to 25 changes to the public Java API that will affect end users.
If there are no changes, you must explicitly state that there are no changes to the public Java API in this PR.
The public Java API is defined as public and protected methods on public classes, plus the filemodule-info.java.
Provide the list by deeply analysing code flow, which incudes analysing code flow through private methods and calls to Guava and Java 21.
Changes to be reported on include:- New or removed methods in the public Java API
- Changes to method return types or parameter types in the public Java API
- Changes to method behaviour in the public Java API that might affect consumers
- This project uses
System.out.printlninstead of logging - This project tends to prefer
forloops to streams for performance reasons, however either form is acceptable.
Do not make suggestions to change between streams and for loops or vice versa.
🔇 Additional comments (17)
src/main/java/org/joda/beans/ResolvedType.java (8)
Line range hint
333-346: Use offrom(Class<?> rawType)ensures safe defaults
The newfrom(Class<?> rawType)method’s approach of defaulting missing type arguments toObjectis clear and intuitive. It avoidsnulltype arguments while preserving backward compatibility.
350-364: Ensure consistent usage of newfrom(Type javaType, Class<?> contextClass, boolean allowRaw)
The refactored code introduces an additional boolean parameterallowRaw. Verify that all callers in the codebase understand the difference between allowing raw types and enforcing default type arguments, to avoid unexpected behaviour.
365-382: Check for potential side effects ofallowRaw
WhenallowRawis set to true, type variables might be left unresolved instead of defaulting toObject. Ensure that all downstream code that processes theseResolvedTypeobjects can handle incomplete type arguments.
417-421: Preferred usage ofvar
In the loop that populatestypeArguments, the code usesfor (var i = 0; ...). This aligns well with Java 21 guidelines to streamline type declarations. Nicely done.
610-623: Implementation oftoJavaType()
The new logic for arrays and parameterised types is well-structured. Wrapping parameterised types withDynamicParameterizedTypeor arrays withDynamicGenericArrayTypeensures correctTyperepresentations.
Line range hint
625-647: Methods for array handling
toComponentType()andtoComponentTypeOrDefault()gracefully handle arrays, providing error prevention for non-array types. This clarity improves maintainability.
671-683:toBaseType()for dropping array dimensions
This method is useful for obtaining the non-array class while preserving generic arguments. Make sure that upstream code is tested with multi-dimensional arrays to confirm correctness.
827-859:DynamicGenericArrayType
Similarly, this class helps represent generic arrays. Confirm no reflection-based logic incorrectly identifies multi-dimensional arrays as single-dimensional, and ensure test coverage of complex generic array scenarios.src/test/java/org/joda/beans/TestResolvedType.java (4)
71-72: Raw vs. parameterised usage
The calls toResolvedType.of(List.class)with no arguments vs.ResolvedType.from(List.class)highlight the subtle distinction between raw types and defaulted arguments. These tests effectively reveal any pitfalls in raw-type handling.
94-96: Testing arrays of collections
TestingList[].classhelps ensure array-of-generic classes remain accurate through the new logic inResolvedType, including correct string representations and argument forwarding.
245-254:test_queries
Lines 245-254 introduce checks forisArrayandisRaw. The approach of peeling off each array dimension to test the underlying raw type is correct. This ensures robust coverage of multi-dimensional arrays.
375-386: Refined dynamic type tests
These lines introduce fields (field0tofield3b) as test fixtures for raw and parameterised array types, plusOptional. The checks intest_dynamicTypesconfirm that no unexpected resolution behaviour creeps in. This is thorough and good practice.Also applies to: 390-401, 403-415
src/main/java/org/joda/beans/ser/bin/JodaBeanStandardBinWriter.java (2)
272-272:toWeakenedTypeshift fromtoRawType()totoRaw()
Returningbase.toRaw()now ensures the raw type is used for collections. This helps unify logic with other changes inResolvedType. Confirm that older code references have been fully updated.
670-670:OptionalBinHandlerrespects raw argument
UsingdeclaredType.getArgumentOrDefault(0).toRaw()for Optionals ensures consistent serialisation of the underlying type. This is a positive move for uniformity.src/main/java/org/joda/beans/ser/json/JodaBeanJsonWriter.java (3)
276-276: Fetching resolved type
ReplacingmetaProperty.propertyGenericType(...)withmetaProperty.propertyResolvedType(...)ensures that the new advanced type resolution is used. This helps unify JSON serialisation with the same logic as binary serialisation.
481-481: Use oftoRaw()with nested collections
Switching fromtoRawType()totoRaw()keeps the code aligned with the updatedResolvedType. This ensures no partial type arguments remain inadvertently.
898-898:OptionalJsonHandlerandGuavaOptionalJsonHandler
Lines 898, 912, 931, and 945 consistently retrievetoRaw()for the contained type and skip writing empty optionals. This matches best practices for consistency and simpler JSON.Also applies to: 912-912, 931-931, 945-945
* Allow a `ResolvedType` to be converted to a `Type` * Rename methods and add Javadoc for clarity * Fix uses of resolved type in Binary/JSON serialization
jodastephen
force-pushed
the
resolved-java-type
branch
from
498679a to
326c78e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/java/org/joda/beans/TestResolvedType.java (1)
376-414: Dynamic type tests are comprehensive, consider adding more edge cases.The test coverage is good, but consider adding test cases for:
- Nested generic types (e.g.,
List<Map<String, Integer>>)- Wildcard types with bounds
src/main/java/org/joda/beans/ResolvedType.java (2)
761-822: DynamicParameterizedType implementation is thorough!Consider adding @OverRide annotation to toString() method for consistency.
- public String toString() { + @Override + public String toString() {
824-858: DynamicGenericArrayType implementation is solid!Consider adding @OverRide annotation to toString() method for consistency.
- public String toString() { + @Override + public String toString() {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/joda/beans/ResolvedType.java(13 hunks)src/main/java/org/joda/beans/ser/bin/JodaBeanStandardBinWriter.java(5 hunks)src/main/java/org/joda/beans/ser/json/JodaBeanJsonWriter.java(6 hunks)src/test/java/org/joda/beans/TestResolvedType.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/joda/beans/ser/bin/JodaBeanStandardBinWriter.java
- src/main/java/org/joda/beans/ser/json/JodaBeanJsonWriter.java
🧰 Additional context used
📓 Path-based instructions (2)
src/test/java/org/joda/beans/TestResolvedType.java (2)
Pattern **/*.java: - Review code using Java 21 standards, taking into account the rules defined by src/main/checkstyle/checkstyle.xml.
- Validate that code indentation uses spaces, not tabs, with an indent of multiple of 4.
- Validate that immutable local variables are not annotated with
finalunless the variable is required for use in an inner class. - Favour use of
varkeyword for type declarations.varmay also be used when the value is a castnull. - Use a coding standard where multi-line expressions have operators and tenary separators at the end of line.
- Propose changes that only use the Java 21 API, not the API of Guava.
- The pattern matching
instanceofexpression safely handlesnull, returningfalse.
Pattern **/test/java/**/*.java: For test code, focus on:
- Correctness of test assertions
- Test coverage of edge cases
- Clear test naming and documentation
- Encourage test methods to be package-scoped where possible
- Be more lenient with code style and minor optimisations
src/main/java/org/joda/beans/ResolvedType.java (2)
Pattern **/*.java: - Review code using Java 21 standards, taking into account the rules defined by src/main/checkstyle/checkstyle.xml.
- Validate that code indentation uses spaces, not tabs, with an indent of multiple of 4.
- Validate that immutable local variables are not annotated with
finalunless the variable is required for use in an inner class. - Favour use of
varkeyword for type declarations.varmay also be used when the value is a castnull. - Use a coding standard where multi-line expressions have operators and tenary separators at the end of line.
- Propose changes that only use the Java 21 API, not the API of Guava.
- The pattern matching
instanceofexpression safely handlesnull, returningfalse.
Pattern **/main/java/**/*.java: - This project is mature and must provide a stable backwards-compatible public Java API.
- In the 'Walkthrough' section, you must always provide a list of up to 25 changes to the public Java API that will affect end users.
If there are no changes, you must explicitly state that there are no changes to the public Java API in this PR.
The public Java API is defined as public and protected methods on public classes, plus the filemodule-info.java.
Provide the list by deeply analysing code flow, which incudes analysing code flow through private methods and calls to Guava and Java 21.
Changes to be reported on include:- New or removed methods in the public Java API
- Changes to method return types or parameter types in the public Java API
- Changes to method behaviour in the public Java API that might affect consumers
- This project uses
System.out.printlninstead of logging - This project tends to prefer
forloops to streams for performance reasons, however either form is acceptable.
Do not make suggestions to change between streams and for loops or vice versa.
🔇 Additional comments (9)
src/test/java/org/joda/beans/TestResolvedType.java (4)
71-72: Test case for raw List type looks good!The test case properly verifies the handling of raw List type without type arguments.
94-96: Test case for List array type looks good!The test case properly verifies the handling of List array type without type arguments.
245-254: Array type handling logic looks good!The code correctly extracts the base component type from array types, handling both single and double array dimensions. The isRaw() check is properly updated to use the base component type.
285-297: Test method for toJavaType() is well-structured!The test method effectively verifies:
- Raw type conversion for types without arguments
- Bidirectional conversion between ResolvedType and Java Type
src/main/java/org/joda/beans/ResolvedType.java (5)
Line range hint
332-346: Method update aligns with PR objectives!The method now correctly handles raw type resolution with clear documentation and examples.
348-380: API design for type resolution is well-structured!The separation between from() and fromAllowRaw() provides clear choices for type resolution with excellent documentation.
401-413: Type resolution logic is robust!The method properly handles raw types and type resolution based on the allowRaw flag.
605-622: Type conversion implementation is elegant!The method effectively handles conversion to Java Type for both simple types and complex generic/array types.
670-682: Base type extraction is well-implemented!The method effectively extracts base types from array types with clear documentation and examples.
ResolvedTypeto be converted to aTypeSummary by CodeRabbit
Release Notes
New Features
Improvements
Testing
These updates improve the library's type handling and provide more robust type resolution mechanisms.