search

oborel / obo-relations

RO is an ontology of relations for use with biological ontologies
http://oborel.github.io/
Other
91 stars 46 forks source link

Revert previous committed changes made in PR #672 and add missing axioms for `has roost` #730

Closed wdduncan closed 1 year ago

wdduncan commented 1 year ago

Reverts oborel/obo-relations#672 Fixes #732

wdduncan commented 1 year ago

@matentzn I've done the revert

anitacaron commented 1 year ago

Why was this PR not merged? This is fixing all issues!

wdduncan commented 1 year ago

There has been a lot of activity. I don't know if this PR is needed.

gouttegd commented 1 year ago

@wdduncan My understanding is that #672, in addition to introducing breaking changes (invalid changes that were correctly detected by the test suite and that prevented RO from being successfully built), also introduced other changes that didn’t trigger any failure in the test suite (they didn’t violate any rules) but were nonetheless incorrect.

The breaking changes were all fixed by by #731 (allowing all CI checks to pass), but the other, non-breaking changes, being undetectable by the test suite, were not. This is what @anitacaron is now addressing by re-opening this PR.

wdduncan commented 1 year ago

Thanks @gouttegd

anitacaron commented 1 year ago

All the work done didn't revert any of the reverts that this PR is doing. For instance, this rollback of the changes made in the PRs since Jan 18 because of commit https://github.com/oborel/obo-relations/pull/672/commits/875a4572a8213197c86e548dff97dba72b69aa54 made without updating first from the master.

matentzn commented 1 year ago

This is really really really horrendous.. I hope with the new branch protection rules this will never happen again.

gouttegd commented 1 year ago

I hope with the new branch protection rules this will never happen again.

The kind of unwanted changes here (such as the revert to CARO terms instead of UBERON terms in some places) would not have been caught by the CI checks, so merely preventing merging until CI checks have passed will not prevent that.

anitacaron commented 1 year ago

There're other preventions in place now. The PR needs two approvals, and a new commit after approval will dismiss it.