Skip to content
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

Remove isPresent check when choosing whether to create a ListNode #15

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pfroud
Copy link
Contributor

@pfroud pfroud commented Jun 28, 2022

See #14

Copy link
Owner

@Mingun Mingun left a comment

Choose a reason for hiding this comment

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

You removed too much -- only isPresent check should gone (and also you should change how to access to the array items spans is implemented, because value instance will have not entries in arrStart / arrEnd). Currently you replaced local span variable by this.span field (defined in super class ChunkNode).

@pfroud
Copy link
Contributor Author

pfroud commented Jun 28, 2022

I see what you mean, it was more complicated than I thought. I put the local variable span back in

@Mingun
Copy link
Owner

Mingun commented Jun 29, 2022

I think that you need to add a new InstanceListNode which will not require a span information, then create it instead of ListNode when isPresent is false. Do not forgot to update diagram in readme.

@pfroud
Copy link
Contributor Author

pfroud commented Jun 29, 2022

If we added InstanceListNode, I think it would then make sense to add InstanceStructNode and InstanceSimpleNode. But there is already ChunkNode.isSequential to tell if it's an instance or not.

@Mingun
Copy link
Owner

Mingun commented Jun 29, 2022

Maybe. Need to trial and see

@pfroud
Copy link
Contributor Author

pfroud commented Jul 13, 2022

I pushed the test which I originally used to find this issue.

The KSY file: https://github.com/pfroud/kaitai_struct_gui/blob/master/src/test/resources/ksy/basic.ksy

The test class: https://github.com/pfroud/kaitai_struct_gui/blob/master/src/test/java/BasicTest.java

The only tests that fail are for value instances where the value is a list. kaitai-struct-model produces a SimpleNode but the tests expects a ListNode.

BasicTest.valueInstanceBooleanList:210->AbstractTest.assertEnumsForNode:66 The node type SimpleNode, the value is type ArrayList ==> expected: <LIST> but was: <SCALAR>
BasicTest.valueInstanceBytesList:170->AbstractTest.assertEnumsForNode:66 The node type SimpleNode, the value is type ArrayList ==> expected: <LIST> but was: <SCALAR>
BasicTest.valueInstanceEnumList:200->AbstractTest.assertEnumsForNode:66 The node type SimpleNode, the value is type ArrayList ==> expected: <LIST> but was: <SCALAR>
BasicTest.valueInstanceFloatList:190->AbstractTest.assertEnumsForNode:66 The node type SimpleNode, the value is type ArrayList ==> expected: <LIST> but was: <SCALAR>
BasicTest.valueInstanceIntegerList:180->AbstractTest.assertEnumsForNode:66 The node type SimpleNode, the value is type ArrayList ==> expected: <LIST> but was: <SCALAR>
BasicTest.valueInstanceStringList:220->AbstractTest.assertEnumsForNode:66 The node type SimpleNode, the value is type ArrayList ==> expected: <LIST> but was: <SCALAR>
BasicTest.valueInstanceStructList:230->AbstractTest.assertEnumsForNode:66 The node type SimpleNode, the value is type ArrayList ==> expected: <LIST> but was: <SCALAR>

it is already possible to distinguish sequential items from instance items, so we do not need to add an InstanceListNode class.

pfroud pushed a commit to pfroud/kaitai_struct_gui that referenced this pull request Oct 15, 2022
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