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

Import OMO terms #3092

Merged
merged 4 commits into from
Oct 26, 2023
Merged

Import OMO terms #3092

merged 4 commits into from
Oct 26, 2023

Conversation

anitacaron
Copy link
Collaborator

@anitacaron anitacaron commented Oct 13, 2023

Fixes #3058

Updated imports/merged_import.owl with OMO:0003004, OMO:0003000 and OMO:0003011.

@anitacaron anitacaron self-assigned this Oct 13, 2023
@anitacaron anitacaron requested a review from a user October 13, 2023 16:19
@matentzn
Copy link
Contributor

Ehm.. a bit too much.. the diff.

@gouttegd
Copy link
Collaborator

The diff is huge because many terms are removed from the import module (especially a bunch of GO terms). Not sure why.

It may have been a glitch of some sort, because I just tried running refresh-imports again and all those terms are back.

@anitacaron
Copy link
Collaborator Author

@matentzn @gouttegd could you review, please?

Copy link
Collaborator

@gouttegd gouttegd left a comment

Choose a reason for hiding this comment

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

I see no problems in the final state of the PR. However I advise against merging it as it is.

In commit 2a10fb6 a lot of terms were removed from the merged_import.owl module; then in the more recent 3240ea7 commit, all those terms were added back. I am not sure why this happened, but there must have been a glitch in the import pipeline when the files that were committed in 2a10fb6 were generated.

So now the final state of merged_import.owl is good, but having two “opposite” commits (one that removes ~100K lines for nothing, and then a second that adds those ~100K lines back) would needlessly pollute the history and I advise against that.

Either:

  • resubmit a clean PR without the “removal-then-reinsertion” dance, or
  • at the very least make a “squash” merge.

@anitacaron
Copy link
Collaborator Author

It's because of the change in GO import.

@gouttegd
Copy link
Collaborator

gouttegd commented Oct 26, 2023

(Changing my review from “request change” to “approve”, so that you can do a “squash merge” if you decide to go that route.)

@anitacaron
Copy link
Collaborator Author

Yes, squash and merge is the default option when merging a PR.

@anitacaron
Copy link
Collaborator Author

Thanks, Damien.

@gouttegd
Copy link
Collaborator

Yes, squash and merge is the default option when merging a PR.

Err, not here. For me the default option is always “create a merge commit“. Is that something you configured on your GitHub account or something?

@anitacaron anitacaron merged commit 3d04ccf into master Oct 26, 2023
1 check passed
@anitacaron
Copy link
Collaborator Author

I'm not sure.

@matentzn
Copy link
Contributor

This is configured at GitHub project level

@gouttegd
Copy link
Collaborator

But then how comes that it does not apply to me? When I merge a PR on Uberon, for me the default option is always a “normal” merge, not a “squash” one.

@anitacaron anitacaron deleted the anitacaron/issue3058 branch January 24, 2024 10:26
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.

Add OMO import to UBERON
3 participants