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

Bump curation version to v0.26.0 (SCRUM-3367) #1130

Merged
merged 4 commits into from
Oct 24, 2023

Conversation

mluypaert
Copy link
Member

No description provided.

@markquintontulloch
Copy link
Member

Do we really want to change diseaseRelation to relation everywhere? This will presumably require UI changes too. Or do we just want to change it in the indexer where pulling the information from the curation system and creating the search documents?

@mluypaert
Copy link
Member Author

Do we really want to change diseaseRelation to relation everywhere? This will presumably require UI changes too. Or do we just want to change it in the indexer where pulling the information from the curation system and creating the search documents?

Well why did we change the property name but not the display name in the curation application? Changing backend name but not display name is an anti-pattern to my opinion and should be avoided unless there is a good reason to.

@markquintontulloch
Copy link
Member

Do we really want to change diseaseRelation to relation everywhere? This will presumably require UI changes too. Or do we just want to change it in the indexer where pulling the information from the curation system and creating the search documents?

Well why did we change the property name but not the display name in the curation application? Changing backend name but not display name is an anti-pattern to my opinion and should be avoided unless there is a good reason to.

We changed the property name in the curation system because it was at odds with the LinkML model. The search documents do not follow the LinkML model. I agree that consistency is good, but think that this requires a bit more thought as to what we change and when. For example, why replace diseaseRelationNegation with relationNegation when this is something that is constructed for the search document, has nothing to do with the LinkmL model, and will probably result in more UI failures if changed?

@markquintontulloch
Copy link
Member

diseaseRelationNegation is constructed from the negated field and the relation field. It is not part of the LinkML model and, since it is specific to disease annotations, I don't see any compelling reason to change the name. From a quick check, the UI code only seems to refer to diseaseRelationNegation and never directly to diseaseRelation/relation. As such, I'd be inclined to keep diseaseRelationNegation as it is so that this doesn't cause breaking changes in the UI repo.

That said, I'm not particularly familiar with the code in either this repo or the UI one so I will leave it to @cmpich or @oblodgett to approve this PR or request changes.

@mluypaert
Copy link
Member Author

Alright, thanks, mind approving the UI PR as well @oblodgett? (alliance-genome/agr_ui#1213)

@mluypaert mluypaert merged commit cb5ddff into stage Oct 24, 2023
3 checks passed
@mluypaert mluypaert deleted the SCRUM-3367_curation-v0.26.0 branch October 24, 2023 18:10
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.

3 participants