-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add tests to reproduce #5152 (Support iPhone
properties)
#5166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* </ul> | ||
* <p> | ||
* This feature is disabled by default for backward compatibility in Jackson 2.x version and | ||
* will be enabled by default in Jackson 3.0 version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs discussion
Surprisingly setting default to true
does not break anything so I figured enable by default in Jackson 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds support for mixed-capitalization property names (e.g. iPhone
) by introducing a new naming feature and corresponding logic.
- Introduces
MIXED_CAPS_PROPERTY_NAMING
inMapperFeature
- Refactors naming in
DefaultAccessorNamingStrategy
viahandlePropertyName
and addsmixedCapsManglePropertyName
- Adds
IPhoneStyleProperty5152Test
with cases foriPhone
,dLogHeader
, andKBSBroadCasting
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/main/java/com/fasterxml/jackson/databind/MapperFeature.java | Adds MIXED_CAPS_PROPERTY_NAMING enum constant with doc describing its behavior |
src/main/java/com/fasterxml/jackson/databind/introspect/DefaultAccessorNamingStrategy.java | Refactors per-accessor naming into handlePropertyName and implements mixed-caps logic |
src/test/java/com/fasterxml/jackson/databind/introspect/IPhoneStyleProperty5152Test.java | New tests for mixed-cap property naming |
Comments suppressed due to low confidence (1)
src/main/java/com/fasterxml/jackson/databind/introspect/DefaultAccessorNamingStrategy.java:257
- The branch for preserving case when more than two uppercase letters is not covered by existing tests; consider adding a unit test for a method like getURL to ensure original casing is preserved.
int upperCount = 2; // We already know first two are uppercase
src/test/java/com/fasterxml/jackson/databind/introspect/IPhoneStyleProperty5152Test.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/introspect/DefaultAccessorNamingStrategy.java
Outdated
Show resolved
Hide resolved
Ok, quick note: this is not quite along the line I think it could be made to work.
but expecting different logical name in JSON ("phone", "Phone") and so there won't be a reliable way to support various alternatives when starting mangling from accessor names. Be that as it may, what could be useful as the starting would be creating a set of failing tests, under ".tofix". I think we can agree on cases that should work. |
5152 Add more Capital case possible 5152 Add new MapperFeature 5152 Move handling to separate method Finalize 5152 Add nameValidator check also Fix test cases Add more test case fix-5152 Fix iPhone style
This reverts commit dcb2ac6.
} | ||
|
||
@JsonPropertyOrder({ "4Roses", "$dollar", "_underscore" }) | ||
static class NonLetterFirstCharBean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, these are not valid test cases: these will require annotations (like @JsonProperty
) to be recognized as valid; they will not be auto-detected.
Issue #5152 is specifically about case differences. If there is desire to try to support non-letter-starting property names, let's file different issue.
iPhone
propertiesiPhone
properties)
iPhone
properties)iPhone
properties)
Interestingly, |
Fixes #5152