Add UpperCamelCase EnumNamingStrategy
#4660
Conversation
lbenedetto
changed the title
src/main/java/com/fasterxml/jackson/databind/EnumNamingStrategies.java
Outdated
Show resolved
Hide resolved
|
Sounds good. Timing-wise this might not make it in 2.18 (hence refs to 2.19 are actually good). Regardless, one thing we'd need if not yet done earlier is CLA. It's from https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf and is usually easiest to do by printing, filling & signing, scan/photo, email to |
|
Ok, CI failures for JDK 8, couple of things need to be fixed (no |
| private static String toLowerCase(String string) { | ||
| int length = string.length(); | ||
| StringBuilder builder = new StringBuilder(length); | ||
| for (int i = 0; i < length; i++) { | ||
| builder.append(charToLowerCaseIfUpper(string.charAt(i))); |
There was a problem hiding this comment.
Previous implementation had this strange way of converting to lowercase, but I noticed that the PropertyNamingStrategy implementations are perfectly happy to just use the standard toLowerCase function.
The two approaches handle special (Unicode) characters differently, but I don't think that applies to enum names.
| if (lastSeparatorIdx != -1) { | ||
| if (iterationCnt == 0) { | ||
| out = new StringBuilder(enumName.length() + 4 * UNDERSCORE.length()); | ||
| out.append(enumName.substring(iterationCnt, lastSeparatorIdx).toLowerCase()); |
There was a problem hiding this comment.
Please always pass a Locale to toLowerCase or there will be locale-specific issues.
There was a problem hiding this comment.
See LowerCaseStrategy and UpperSnakeCaseStrategy in PropertyNamingStrategies.
They use toLowerCase() and toUpperCase() without passing in the Locale, which is why I thought I could do the same here.
What Locale should I be passing? Should I update those strategies as well?
There was a problem hiding this comment.
Locale.ROOT is the safe choice. imo the other strategies should use the same
@pjfanning @cowtowncoder WDYT
There was a problem hiding this comment.
I am not sure; I don't actually like the idea of case change being contextual since that can lead to interoperability problems; similar to using local timezone for date/time values.
But I am not 100% sure how others deal with this.
So how about this: in 2.x, follow existing (not best) Practice of not passing Locale.
And then file follow-up issue to change this in 3.0 (master) for all casing changes in jackson-databind.
There was a problem hiding this comment.
Actually, reading Locale Javadocs, I agree with @yawkat -- Locale.ROOT would make sense.
Any changes to classes outside EnumNamingStrategy should go in separate PR tho, fwtw. But assuming safe (Locale.ROOT is defaulted to anyway?) could still go in 2.18.
There was a problem hiding this comment.
I'll change it -- and no, default is "local Locale" otherwise.
But one big challenge if changing to configurable: there's no access to configuration from EnumNamingStrategy.convertEnumToExternalName() -- unlike PropertyNamingStrategy which takes MapperConfig
Should change that for 3.0; will create an issue.
src/main/java/com/fasterxml/jackson/databind/PropertyNamingStrategies.java
Outdated
Show resolved
Hide resolved
cowtowncoder
changed the title
UpperCamelCase EnumNamingStrategy
|
Ok LGTM; will merge to get it out of the way. Thank you @lbenedetto ! |
| * An implementation of {@link EnumNamingStrategy} that converts enum names in the typical upper | ||
| * snake case format to upper camel case format. | ||
| * This implementation first normalizes to lower camel case using (see {@link LowerCamelCaseStrategy} for details) | ||
| * and then uses {@link PropertyNamingStrategies.UpperCamelCaseStrategy} to finish converting the name. |
There was a problem hiding this comment.
Uhhh. BAD idea. I wish I had noticed before approving -- should not plug in to internal details of another class.
Just noticed when trying to merge into 3.0 (master).
There was a problem hiding this comment.
I will need to revert this, actually.
There was a problem hiding this comment.
Sorry, I thought it made sense to reuse this preexisting battle-tested implementation for converting lowerCamelCase into other specific cases.
Do you have a recommendation for how this should be achieved instead? Access the same code in a different way? Implement my own logic from scratch? Copy paste?
There was a problem hiding this comment.
Right, I agree with the intent here, just not the mechanism.
Not 100% sure of a good way (can't think of a great way at least): copy pasting (with a comment pointing to source) might be ok here, given it shouldn't change a lot.
Otherwise a util class would be good. Unfortunately there isn't any obvious choice... closest might be BeanUtil, if method(s) in question can be static.
There was a problem hiding this comment.
... also, adding something like NamingUtil in src/main/java/com/fasterxml/jackson/databind/util would be ok if that makes more sense. It's bit dubious given only 2 use cases but... maybe that's the way to do it.
Apologies for catching this bit late.
There was a problem hiding this comment.
Here is the updated pull request:
#4728
This reverts commit 5bf17d9.
This reverts commit 5bf17d9.
|
Ok my apologies: I should have noticed one thing -- implements should NOT use internal implementation of EDIT: as per my comments above, I do not object to reuse of proven code but rather the specific way it was done here. So some refactoring would be needed. And with that it'd be perfectly fine. |
4 participants
Changes EnumNamingStrategies to provide all the same naming strategies as PropertyNamingStrategies. It accomplishes this by first normalizing the enum name to lowerCamelCase and then delegating to the various PropertyNamingStrategies implementations