IonStructLite bugfixes #155
Conversation
…onstruction, added a boolean hasNullField to throw correctly when a fieldname does not resolve and correctness cannot be determined.
| @@ -402,9 +401,9 @@ public IonValue get(String fieldName) | |||
| IonValue field; | |||
|
|
|||
| if (field_idx < 0) { | |||
| if(hasNullFieldName) throw new UnknownSymbolException("Because a field exists with an unknown name we cannot say for certain whether the requested field exists or not."); | |||
There was a problem hiding this comment.
This condition needs a test.
There was a problem hiding this comment.
Also, the message is a bit wordy. The use of 'we', at least, feels too conversational for an exception message to me.
There was a problem hiding this comment.
agreed ill add one.
| @@ -738,6 +738,29 @@ public void testMakeNullRemovesChildsContainer() | |||
| val.getContainer()); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testMapAfterClone() | |||
There was a problem hiding this comment.
Does this actually add coverage?
There was a problem hiding this comment.
Not really its more trying to check the cloning behavior between the container version and Hashmap version of IonStructLite.
There was a problem hiding this comment.
It doesn't really have a way of differentiating between that internal behavior, though. If it's not adding anything, I'd vote to remove it, as there's already plenty of testing to verify that IonStructLite works correctly.
|
|
||
| public UnknownSymbolException(int sid) | ||
| { | ||
| mySid = sid; | ||
| myText = null; | ||
| } | ||
| public UnknownSymbolException(String text) |
There was a problem hiding this comment.
I'd change the parameter to 'message' and in getMessage just return it as-is. Because what's being passed in isn't actually symbol text. This constructor should be used when an unknown symbol causes a problem, but we don't know its symbol ID (as in the case of structs with unknown field names).
There was a problem hiding this comment.
isnt it the symbol text?
There was a problem hiding this comment.
What symbol would it be the text for? If you look at the only place you're actually using it, it's just a message.
| @@ -450,11 +449,6 @@ public boolean add(IonValue child) | |||
| { | |||
| // TODO validate in struct.setFieldName | |||
| String text = child.getFieldNameSymbol().getText(); | |||
| if (text != null) | |||
| { | |||
| validateFieldName(text); | |||
| @@ -483,6 +477,7 @@ protected void handle(IonValue newValue) | |||
| */ | |||
| private void _add(String fieldName, IonValueLite child) | |||
| { | |||
| if(!hasNullFieldName && fieldName == null) hasNullFieldName = true; | |||
There was a problem hiding this comment.
Is there any benefit to checking !hasNullFieldName here? If it's already true, it's not harmful to set it to true.
There was a problem hiding this comment.
it will shortcircuit and prevent repeatedly setting hasNullFieldName
There was a problem hiding this comment.
Who cares if you repeatedly set it though?
There was a problem hiding this comment.
people paying for those clock cycles.
There was a problem hiding this comment.
A branch operation is probably similar in cost to (or even more expensive than) writing to an in-memory primitive with a known address. Both, though, are pretty trivial.
If you're really concerned with clock cycles, you can replace the whole if statement with:
hasNullFieldName |= fieldName == null;😄
| @@ -401,7 +401,7 @@ public IonValue get(String fieldName) | |||
| IonValue field; | |||
|
|
|||
| if (field_idx < 0) { | |||
| if(hasNullFieldName) throw new UnknownSymbolException("Because a field exists with an unknown name we cannot say for certain whether the requested field exists or not."); | |||
| if(hasNullFieldName) throw new UnknownSymbolException("Unable to resolve fieldname due to null fields."); | |||
There was a problem hiding this comment.
'Null fields' is what's happening within the implementation, but from an Ion perspective isn't the issue. I don't know if this is any less wordy than the original, but here's my stab at it:
"Unable to determine whether the field exists because the struct contains field names with unknown text."
Description of changes:
Updated ionstructlite so fieldmap will not always resolve as null on construction, added a boolean hasNullField to throw correctly when a fieldname does not resolve and correctness cannot be determined.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.