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.

Sign up for GitHub

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

Jump to bottom

Fix implicitly bound resources to be able to reference MultiVector #18

Merged
merged 5 commits into from
Feb 12, 2018
Merged

Fix implicitly bound resources to be able to reference MultiVector #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
49e19c3
Fix implicitly bound resources to be able to reference MultiVector
VeaaC Feb 4, 2018
46226c4
Merge branch 'master' into fix_implicit_bound_for_multivector
VeaaC Feb 4, 2018
f821911
Merge branch 'master' into fix_implicit_bound_for_multivector
boxdot Feb 4, 2018
aad94b7
Merge branch 'master' into fix_implicit_bound_for_multivector
VeaaC Feb 12, 2018
cc1ed1f
Merge branch 'master' into fix_implicit_bound_for_multivector
VeaaC Feb 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions generator/tree/nodes/resources/bound_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from .base import ResourceBase
from generator.tree.nodes.references import VectorReference
from generator.tree.nodes.references import ResourceReference


class BoundResource(ResourceBase):
Expand All @@ -15,9 +15,9 @@ def create(properties, own_schema):
own_schema=own_schema)

def _create_references(self):
return [VectorReference(name=r) for r in self._resources]
return [ResourceReference(name=r) for r in self._resources]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not add ResourceReference here as it incldues raw_data and we surely can't implicitly bind to raw_data. How about adding SequentialResourceReference or something like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking about that, and my reasoning was like this:

  • RAW means the user manages the data
  • The user might store data in RAW that use indexed
  • The user might want to indicate in the schema that their RAW data is implicitly bound (however they manage their RAW data)

No strong opinions in that direction, but that seemed like a reasonable thing to allow (we do not benefit from being restrictive, do we?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a small note: Right now, we already allow to create explicit references from vectors into raw data.


@property
def referenced_structures(self):
return [s for r in self.children_like(VectorReference) for s in
return [s for r in self.children_like(ResourceReference) for s in
r.node.referenced_structures]
9 changes: 5 additions & 4 deletions tests/tree/test_syntax_tree_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ def test_duplicate_multivector_builtin_types_are_not_produced():
@bound_implicitly( b: A0.v0, A0.v1 )
archive A0 {
v0 : vector< S1 >;
v1 : vector< S1 >;
v1 : multivector< 14, S1 >;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add, not replace. Better - add another test. This one will check for vectors. Another - for multivectors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is not a test for vector. This tree is named TREE_WITH_ALL_FEATURES

}

const u32 C = 0xFFFFFFF;
Expand Down Expand Up @@ -168,11 +168,12 @@ def test_all_flatdata_features_look_as_expected_in_fully_built_tree():
'.ns.A0': Archive,
'.ns.A0.@@ns@C': ConstantReference,
'.ns.A0.b': BoundResource,
'.ns.A0.b.@@ns@A0@v0': VectorReference,
'.ns.A0.b.@@ns@A0@v1': VectorReference,
'.ns.A0.b.@@ns@A0@v0': ResourceReference,
'.ns.A0.b.@@ns@A0@v1': ResourceReference,
'.ns.A0.v0': Vector,
'.ns.A0.v0.@@ns@S1': StructureReference,
'.ns.A0.v1': Vector,
'.ns.A0.v1': Multivector,
'.ns.A0.v1.@@_builtin@multivector@IndexType14': BuiltinStructureReference,
'.ns.A0.v1.@@ns@S1': StructureReference,
'.ns.A1': Archive,
'.ns.A1.@@ns@C': ConstantReference,
Expand Down