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

New Element Individuals to handle NONE and NOASSERTION scenarios for the 'to' property of Relationships #629

Merged
merged 6 commits into from
Apr 3, 2024

Conversation

sbarnum
Copy link
Collaborator

@sbarnum sbarnum commented Feb 5, 2024

Addresses #527

Copy link
Contributor

@kestewart kestewart left a comment

Choose a reason for hiding this comment

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

Thanks for pulling this together Sean. Looks good to me.

Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

This looks good to me - The description of the NoAssertion captured all the cases I was looking for.

Thanks @sbarnum

maxhbr
maxhbr previously requested changes Feb 7, 2024
Copy link
Member

@maxhbr maxhbr left a comment

Choose a reason for hiding this comment

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

Added some more explanation, especially that [Element1, NONE] is not allowed. We should make that even more explicit so that validation also catches that.

There is also the case of a Relationship with from=NOASSERTION, relationshipType="ancestorOf", and to=Element1. This should also be explained or should be disallowed. Same for NONE.

model/Core/Classes/Relationship.md Outdated Show resolved Hide resolved
model/Core/Classes/Relationship.md Show resolved Hide resolved
@maxhbr
Copy link
Member

maxhbr commented Feb 13, 2024

I disagree that this is the right direction. I think a to relationship towards an empty list, together with relationship completeness, is a better way to express that information.

@kestewart
Copy link
Contributor

Discussion is we should document from migration guide, how to go this for 3.0. Concern is this with completeness is going to cause more confusion than it resolves. Let's document it, and then decide which PR to apply. ( Documenting Empty list is ok with completeness indicator.. vs. adding this in again)

@goneall
Copy link
Member

goneall commented Feb 15, 2024

Decided to move this to post RC2 and go with the existing solution using the completness property for RC2.

@goneall goneall modified the milestones: 3.0-rc2, 3.0 Feb 15, 2024
@kestewart
Copy link
Contributor

kestewart commented Mar 28, 2024

Discussed #623 with @JPEWdev and agreed we're going to merge it. We're going to raise this in next tech call.

@goneall
Copy link
Member

goneall commented Apr 2, 2024

From tech call on 2 April 2024: agreed to rename None and NoAssertion to NoneElement and NoAssertionElement resp.

model/Core/Classes/Relationship.md Outdated Show resolved Hide resolved
model/Core/Classes/Relationship.md Outdated Show resolved Hide resolved
Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

Per tech call on 2 April 2024 -

@goneall goneall force-pushed the none-noassertion-relationships branch from 704fe7a to dd15fb3 Compare April 2, 2024 17:28
@goneall
Copy link
Member

goneall commented Apr 3, 2024

@maxhbr - I believe this matches our discussion on the tech call - please re-review

@maxhbr maxhbr dismissed their stale review April 3, 2024 13:52

The review is stale

@goneall goneall merged commit eb10c88 into main Apr 3, 2024
1 check passed
@goneall goneall deleted the none-noassertion-relationships branch April 3, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants