-
Notifications
You must be signed in to change notification settings - Fork 35
Add support for short/float/double arrays #197
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
base: 2.x
Are you sure you want to change the base?
Conversation
@@ -61,6 +61,9 @@ public abstract class ValueLocatorBase | |||
public final static int SER_INT_ARRAY = 5; | |||
public final static int SER_LONG_ARRAY = 6; | |||
public final static int SER_BOOLEAN_ARRAY = 7; | |||
public final static int SER_SHORT_ARRAY = 38; |
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 to be consecutive.
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.
... which gets tricky wrt whether to renumber entries or move these to later.
I think renumbering is needed, so move SER_TREE_NODE
etc up by number of added entries needed.
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.
Yes, I saw that, and had the same thought, but I figured you'd know which of the two options (renumbering all later constants, or moving the other arrays to the end, which is ugly) would make the most sense.
@lukehutch Did you review it yourself? Or run tests? (although TBH Sonatype Snapshot repo has issues so perhaps it is difficult). Looks like unit tests fail. Overall makes sense, GenAI coding agents are pretty good at following patterns. But there are some aspects that need changing; adding comments. |
p.nextToken(); | ||
} | ||
|
||
java.util.List<Long> values = new java.util.ArrayList<>(); |
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.
Very inefficient, no. Have a look at how int[]
is handled.
case JsonTokenId.ID_END_ARRAY: | ||
break main_loop; | ||
default: | ||
throw new JSONObjectException("Failed to bind `long` element if `long[]` from value: "+ |
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.
Funny. Copied typo from int[]
case :)
("if" -> "of")
(will fix separately)
p.nextToken(); | ||
} | ||
|
||
java.util.List<Boolean> values = new java.util.ArrayList<>(); |
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.
Same as above, need to build an array to avoid all the boxing, unboxing.
_generator.writeFieldName(fieldName); | ||
writeDoubleArrayValue(v); | ||
} | ||
|
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.
This class looks good.
I couldn't run the tests, and yes, this is going to be imperfect, but I figured I'd submit it as a starting point to save you a lot of work. Thanks! |
By the way you could also continue to iterate with Claude Code -- probably the issues you found could be fixed with a couple more prompts consisting of one or two sentences. |
My experience with Wind Surfer was... not great so I am bit of GenAI skeptic. While tools are impressive in their own right, the one and only piece of code in Jackson that I auto-generated, turned out to be the first bug reported for 2.19.0.... :-D |
Windsurf is pretty bad. Claude Code is almost magical, especially for tasks like this that really just require pattern matching, as long as the codebase is not enormous (in which case you run into context window size issues, where it will forget state each time it has to compact the context). |
@lukehutch were you able to use Claude code to look at current failures? Since we are at public repository, I suppose it should be able to fetch CI failures as well? |
I didn't have it look at test results or anything like that. I had it identify missing support for arrays of any of the 7 primitive types. It correctly identified that arrays of char are treated as strings, and some other special case handling. |
Proposed fix for #196
Created with Claude Code, so should be double-checked :-)