Skip to content

fixes isInStruct/getFieldName/getFieldNameSymbol behavior for DOM-backed readers#134

Merged
pbcornell merged 2 commits intomasterfrom
is-in-struct-fix
Jan 6, 2018
Merged

fixes isInStruct/getFieldName/getFieldNameSymbol behavior for DOM-backed readers#134
pbcornell merged 2 commits intomasterfrom
is-in-struct-fix

Conversation

@pbcornell
Copy link
Copy Markdown

Fix for #133

@pbcornell pbcornell self-assigned this Dec 19, 2017
@pbcornell pbcornell requested a review from tgregg December 19, 2017 02:21
iw.writeValue(ir);
iw.stepOut();
assertEquals(data, reloadSingleValue());
iw.writeValue(ir); // should throw IllegalStateException ("Field name not set")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been starting to shift to using JUnit's ExpectedException (instead of @Test(expected=...)) so that the particular line where the exception is expected may be isolated.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

public boolean isInStruct()
{
return (_parent instanceof IonStruct);
return getDepth() > 0 && _parent instanceof IonStruct;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a DOM value is provided to a reader, shouldn't the reader's public getDepth() method restart at zero for that value? It should also be impossible to stepOut past that depth.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, getDepth() should start at zero when a reader is constructed over a DOM -- and it already does. This change leverages that fact to fix the behavior of isInStruct() such that it initially returns false when a reader is constructed over a DOM.

And attempts to stepOut() to a parent of the DOM value already results in an IllegalStateException.

For a test (albeit a very terse one) that asserts the expected behavior, see TreeReaderTest.testReadingStructFields().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants