(3.0) NullNode.stringValue() should return null ...
#5288
Conversation
|
Probably worth refining the Javadoc of |
| assertEquals(null, node.stringValue()); | ||
|
|
||
| // But also check defaulting | ||
| assertEquals("foo", node.stringValue("foo")); |
There was a problem hiding this comment.
No: should return null, now that it is legitimate String value.
There was a problem hiding this comment.
I think I am stongly in favor of keeping the PR as it was since null typically express the absence of value. I think the least surprising behavior is the previous PR implementation, with node.stringValue("foo") returning "foo" and node.stringValueOpt().isPresent() returning false when the string is null. Both should work consistently as a default value is supposed to be returned in the absence of value.
There was a problem hiding this comment.
Hmmm okay I am a bit confused now.
Seems like we missed talking what each case should handle defaultValue.
Which one should be?
assertEquals("foo", nullNode.stringValue("foo"));assertEquals(null, nullNode.stringValue("foo"));
Let's continue discussion on #5287
There was a problem hiding this comment.
Hmmh. I guess I changed my mind -- will revert to earlier state. This mostly thanks to Optional limitation.
|
@sdeleuze @JooHyukKim Changed as discussed. Still looking forward to suggestions for better Javadocs but ready to approve if we agree on behavioral change. |
|
LGTM |
|
LGTM as well. |
... as per #5287