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

[Core] Split Specification to data + view #348

Closed
Tracked by #319
foundrytom opened this issue Apr 25, 2022 · 0 comments · Fixed by #373
Closed
Tracked by #319

[Core] Split Specification to data + view #348

foundrytom opened this issue Apr 25, 2022 · 0 comments · Fixed by #373
Assignees
@foundrytom
Labels
api change
Milestone
Cpp in main

Comments

@foundrytom
Copy link
Collaborator

foundrytom commented Apr 25, 2022 •
edited

What

Split Specification into TraitsData and Specification

Why

The two being the same inadvertently creates a logical impossibility - in that, it is suggested that a specification can be constructed by wrapping around a base class, eg: ImageSpecification(specificaion_from_resolve). This has assorted problems:

  • When a specification imbues it's trait IDs to the data is ambigous and can result in inadvertent modification.
  • Can't be achieved without a more decomposed copy in C++`and Python (as you're effectively upcasting).

Notes

  • Specifications are constructed as views on a data, and hold a shared ptr to it
  • Specifications don't imbue traits when constructed
  • Specifications have a imbuedData method that returns a copy of the data with all traits imbued to be passed to register.
  • A default constructed specifications create their own new empty data.

Acceptance Criteria

  • Rename Specification to TraitsData final class
  • Add copy constructor to TraitsData
  • Examples and documentation updated
@foundrytom foundrytom added this to the Core stable milestone May 3, 2022
This was referenced May 3, 2022
Closed
Closed
foundrytom added a commit to foundrytom/OpenAssetIO that referenced this issue May 9, 2022
@foundrytom
[Core] Allow Specification to be constructed with no traits
528c296 
Part of OpenAssetIO#335, and later OpenAssetIO#348.

Removing the requirement for up-front knowledge simplifies the logic in
many common applications, and will better support the 'view/builder'
design of the upcoming Specification/TraitsData split.

Signed-off-by: Tom Cowland <tom@foundry.com>
foundrytom added a commit to foundrytom/OpenAssetIO that referenced this issue May 9, 2022
@foundrytom
[Core] Allow Specification to be constructed with no traits
6ad75a5 
Part of OpenAssetIO#335, and later OpenAssetIO#348.

Removing the requirement for up-front knowledge simplifies the logic in
many common applications, and will better support the 'view/builder'
design of the upcoming Specification/TraitsData split.

Signed-off-by: Tom Cowland <tom@foundry.com>
@foundrytom foundrytom self-assigned this May 10, 2022
foundrytom added a commit to foundrytom/OpenAssetIO that referenced this issue May 12, 2022
@foundrytom
[Core] Split data container from Specification into TraitsData
b1f1960 
The existing approach of Specification being a base class for custom
views _and_ the container holding the data the views operate on created
assorted problems:

- It has too many responsibilities.

- It was expected that default constructed Specification derived classes
  imbued the specification with their defined trait set. Yet, at the same
  time, passing a base class specification into derived one's constructor
  was meant to transparently 'wrap' this data to allow introspection without
  imbuing any new specifications. So the observed behaviour is divergent,
  and counterintuitive.

- The above also implied that it was down-casting the specification
  which, is not the case as the data from resolve etc. is never going
  to be an instance of the more derived class.

- Passing the specification instance into register opened up the
  possibility of someone wrapping resolved data in a view, modifying it,
  then passing this back to register. If the resolved data was missing
  any of the traits of that specification, they would never be imbued as
  they are only added to default constructed versions.

This commit separates out the data from the view.

 - TraitsData is the transport-layer container for a trait set and any
   trait property values.

 - Specification becomes the base class for strongly-typed views on
   an a TraitsData instance.

This resolves most the majority of the above concerns, and better
reenforces the requirement for the low-level transport of API data to
be simple and generic.

We spent a suitable amount of time going around in circles, and naming
wise, TraitsData won out over TraitData, TraitsInstance,
SpecificationData and values as it best defines its applicability is
limited to traits, and not all traits have values.

Closes OpenAssetIO#348.

Signed-off-by: Tom Cowland <tom@foundry.com>
@foundrytom foundrytom mentioned this issue May 12, 2022
Merged
foundrytom added a commit to foundrytom/OpenAssetIO that referenced this issue May 13, 2022
@foundrytom
[Core] Allow Specification to be constructed with no traits
4b39e30 
Part of OpenAssetIO#335, and later OpenAssetIO#348.

Removing the requirement for up-front knowledge simplifies the logic in
many common applications, and will better support the 'view/builder'
design of the upcoming Specification/TraitsData split.

Signed-off-by: Tom Cowland <tom@foundry.com>
@foundrytom foundrytom modified the milestones: Core stable, Cpp in main May 16, 2022
foundrytom added a commit to foundrytom/OpenAssetIO that referenced this issue May 16, 2022
@foundrytom
[Core] Split data container from Specification into TraitsData
a34e745 
The existing approach of Specification being a base class for custom
views _and_ the container holding the data the views operate on created
assorted problems:

- It has too many responsibilities.

- It was expected that default constructed Specification derived classes
  imbued the specification with their defined trait set. Yet, at the same
  time, passing a base class specification into derived one's constructor
  was meant to transparently 'wrap' this data to allow introspection without
  imbuing any new specifications. So the observed behaviour is divergent,
  and counterintuitive.

- The above also implied that it was down-casting the specification
  which, is not the case as the data from resolve etc. is never going
  to be an instance of the more derived class.

- Passing the specification instance into register opened up the
  possibility of someone wrapping resolved data in a view, modifying it,
  then passing this back to register. If the resolved data was missing
  any of the traits of that specification, they would never be imbued as
  they are only added to default constructed versions.

This commit separates out the data from the view.

 - TraitsData is the transport-layer container for a trait set and any
   trait property values.

 - Specification becomes the base class for strongly-typed views on
   an a TraitsData instance.

This resolves most the majority of the above concerns, and better
reenforces the requirement for the low-level transport of API data to
be simple and generic.

We spent a suitable amount of time going around in circles, and naming
wise, TraitsData won out over TraitData, TraitsInstance,
SpecificationData and values as it best defines its applicability is
limited to traits, and not all traits have values.

Closes OpenAssetIO#348.

Signed-off-by: Tom Cowland <tom@foundry.com>
This was referenced May 18, 2022
Closed
Closed
@foundrytom foundrytom added decision record on hold Should not be merged and removed decision record labels May 18, 2022
foundrytom added a commit to foundrytom/OpenAssetIO that referenced this issue May 20, 2022
@foundrytom
[Core] Split data container from Specification into TraitsData
a7661b5 
The existing approach of Specification being a base class for custom
views _and_ the container holding the data the views operate on created
assorted problems:

- It has too many responsibilities.

- It was expected that default constructed Specification derived classes
  imbued the specification with their defined trait set. Yet, at the same
  time, passing a base class specification into derived one's constructor
  was meant to transparently 'wrap' this data to allow introspection without
  imbuing any new specifications. So the observed behaviour is divergent,
  and counterintuitive.

- The above also implied that it was down-casting the specification
  which, is not the case as the data from resolve etc. is never going
  to be an instance of the more derived class.

- Passing the specification instance into register opened up the
  possibility of someone wrapping resolved data in a view, modifying it,
  then passing this back to register. If the resolved data was missing
  any of the traits of that specification, they would never be imbued as
  they are only added to default constructed versions.

This commit separates out the data from the view.

 - TraitsData is the transport-layer container for a trait set and any
   trait property values.

 - Specification becomes the base class for strongly-typed views on
   an a TraitsData instance.

This resolves most the majority of the above concerns, and better
reenforces the requirement for the low-level transport of API data to
be simple and generic.

We spent a suitable amount of time going around in circles, and naming
wise, TraitsData won out over TraitData, TraitsInstance,
SpecificationData and values as it best defines its applicability is
limited to traits, and not all traits have values.

Closes OpenAssetIO#348.

Signed-off-by: Tom Cowland <tom@foundry.com>
foundrytom added a commit to foundrytom/OpenAssetIO that referenced this issue May 20, 2022
@foundrytom
[Core] Split data container from Specification into TraitsData
1c05fa5 
The existing approach of Specification being a base class for custom
views _and_ the container holding the data the views operate on created
assorted problems:

- It has too many responsibilities.

- It was expected that default constructed Specification derived classes
  imbued the specification with their defined trait set. Yet, at the same
  time, passing a base class specification into derived one's constructor
  was meant to transparently 'wrap' this data to allow introspection without
  imbuing any new specifications. So the observed behaviour is divergent,
  and counterintuitive.

- The above also implied that it was down-casting the specification
  which, is not the case as the data from resolve etc. is never going
  to be an instance of the more derived class.

- Passing the specification instance into register opened up the
  possibility of someone wrapping resolved data in a view, modifying it,
  then passing this back to register. If the resolved data was missing
  any of the traits of that specification, they would never be imbued as
  they are only added to default constructed versions.

This commit separates out the data from the view.

 - TraitsData is the transport-layer container for a trait set and any
   trait property values.

 - Specification becomes the base class for strongly-typed views on
   an a TraitsData instance.

This resolves most the majority of the above concerns, and better
reenforces the requirement for the low-level transport of API data to
be simple and generic.

We spent a suitable amount of time going around in circles, and naming
wise, TraitsData won out over TraitData, TraitsInstance,
SpecificationData and values as it best defines its applicability is
limited to traits, and not all traits have values.

Closes OpenAssetIO#348.

Signed-off-by: Tom Cowland <tom@foundry.com>
@foundrytom foundrytom removed the on hold Should not be merged label May 20, 2022
@foundrytom foundrytom linked a pull request May 27, 2022 that will close this issue
Split Specification into views + data #373
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@foundrytom foundrytom

Labels
api change
Projects
OpenAssetIO Development
Status: Done
Milestone
Cpp in main
Development

Successfully merging a pull request may close this issue.

Split Specification into views + data foundrytom/OpenAssetIO
1 participant
@foundrytom