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

Resolve inheritance discrepancies between core-im-source yaml and the original/informal core-im spec. #135

Open
mbrush opened this issue May 28, 2024 · 3 comments
Assignees

Comments

@mbrush
Copy link
Contributor

mbrush commented May 28, 2024

Different inheritance of some classes in the initial core-im-source yaml,compared to inheritance in the original/informal core-im specification. e,g, in the current core-im yaml:

  • Method inherits directly from Entity (instead of InformationEntity)
  • Document inherits directly from MappableEntity (instead of InformationEntity)

If this was done because inheritance from core im classes with their full set of properties was deemed unwanted/problematic in the context of profiles that use these classes to instantiated data – we should consider solutions that are able to maintain the correct/intended hierarchy of classes.

@mbrush mbrush changed the title core-im: Resolve inheritance discrepancies between core-im-source yaml and the original/informal core-im spec. Resolve inheritance discrepancies between core-im-source yaml and the original/informal core-im spec. May 29, 2024
@larrybabb
Copy link
Contributor

I'll defer to @ahwagner on this one. My guess is that Method was an oversight. But I think we intentionally wanted Document to be mappable. Again, this requires you (@mbrush) to discuss with @ahwagner .

@larrybabb
Copy link
Contributor

@mbrush based on our meetings with each other on this topic and my review with @ahwagner . We have adjusted the core-im as follows:

  • Method now inherits from InformationEntity and was moved to the gks_common.core-im
  • Document now inherits from InformationEntity and was moved to the gks_common.core-im
  • Contribution and Agent were moved to gks_common.core-im to support RecordMetadata
  • RecordMetadata was added to the gks_common.core-im and it required visibility to Contribution
  • InformationEntity was moved to the gks_common.core-im as it was referenced by Contribution.contributionMadeTo
  • All DomainEntity subclasses where moved to a separate gks_common.domain-entities subfolder and single domain-entities-source.yaml file
  • vrs.ValueObject now inherits from gks_common.core-im.DomainEntity
  • MappableEntity was renamed to ConceptMapping and the Mappable class was deleted, DomainEntity now has a mappings array of ConceptMapping
  • aliases attributes were all re-aligned to the alternativeLabels attribute on the top-level Entity class

I think there may be more, but the above should have moved us closer to where we want to be.

As far as the gks-common.core-im-source.yaml file, I recognize this could trigger a negative reaction from you as we tried this once before. But I really think we should consider this to be an essential part of the Core Info Model for GKS. Just because these top-level classes are available to all GKS repositories does not change the fact that they are the building blocks upon which the va-spec is able to further build out the core-im for the Statement and StudyResult profiles we are looking to build. I'm sure we will ultimately be moving things around as we all start to see how the GKS repos develop. But I feel that it is important to consider the GKS-common.core-im and va-spec.core-im to be all part of the same thing.

@mbrush
Copy link
Contributor Author

mbrush commented Jul 4, 2024

Thanks for all this work Larry. Most of this i am on board with. A few things I am proposing to change:


InformationEntity was moved to the gks_common.core-im as it was referenced by Contribution.contributionMadeTo

I don't think we need to or want to do this. First, I don't think we should even include the contributionMadeTo property in the model right now. There is no real use case for it, as Contributions are always referenced from the artifacts contributed to, not the other way. But even if we do include this property, I think that in the context of gks commons we want to bind it to Entity rather than InformationEntity - to give it broader utility in describing contributions to a wider range of artifacts.

Given all this, we can keep InformationEntity in the core-im (because it would no longer be referenced by Contribution in gks-common). If we do want the contributionMadeTo property in the core-im, we can always further 'extend' it there to take an InformationEntity as its value. But for now I prefer it in the va-spec core-im.


  1. With InformationEntity back in the va core-im file, there is no need to move Method and Document into gks common (as these will no longer be referenced by any properties in gks-common). We can move these back into va core-im along with InformationEntity, so we can keep the clear dividing line of va-spec starting with InformationEntity and its subtypes.

  1. Finally, since Contribution inherits from Activity, we need to move Activity into gks-commons.

So, I made PR#32, PR#33, and PR#34 in gks-commons, and PR#167 in va-spec to implement the changes needed to achieve all of the above suggestions. @larrybabb we can review next time we talk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

3 participants