Solve ordering issue of record constructor parameters #169
Conversation
Giovds
force-pushed
the
issue-167-solve-ordering-problem-of-record-constructor-params
branch
from
38e022f
to
eb422c6
Compare
Giovds
force-pushed
the
issue-167-solve-ordering-problem-of-record-constructor-params
branch
from
eb422c6
to
16c9ff0
Compare
|
Ok thank you @Giovds ! I'll need to go over this with more thought, but on first look it looks good! One thing I'll need before merging -- if not already done earlier -- is to get a CLA. It needs to be sent just once before first contribution and is good for any later contributions. https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf and the usual ways is to print, fill & sign, scan/photo, email to Looking forward to merging this in! |
|
I've sent the email 👍 |
|
CLA received, hoping to review this soon. |
jr-objects/src/main/java/com/fasterxml/jackson/jr/ob/impl/ValueReaderLocator.java
Show resolved
Hide resolved
| final Parameter[] parameters = constructors._recordCtor.getParameters(); | ||
| for (int i = 0; i < parameters.length; i++) { | ||
| Parameter parameter = parameters[i]; | ||
| if (!parameter.getName().isEmpty()) { |
There was a problem hiding this comment.
Why such check? Does this happen? Isn't it an error condition?
And specifically if this happens wouldn't it just lead to an NPE when looking up the index?
There was a problem hiding this comment.
Actually, I think this is an irrelevant check, as you can not create a (record) instance variable without a name... It would lead to a NPE in the look-up part. I'd say just remove the check, as you can not test this as you'd get a compile error? WDYT?
There was a problem hiding this comment.
Yes, sounds good. But as per my note above, might be this part of code can be removed altogether.
…onstructor-params
…onstructor-params
…onstructor-params
…onstructor-params
|
Happy to see it fixed. Hopefully it was of any help 👍 |
|
@Giovds It definitely was -- thank you for helping here. |
Closes #167