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

Created script to resolve phylorefs using the Open Tree of Life APIs #51

Merged
merged 27 commits into from
Nov 9, 2020

Conversation

gaurav
Copy link
Member

@gaurav gaurav commented Apr 28, 2020

Added a script that can be used to resolve multiple Phyx files against the Open Tree of Life using their updated MRCA API (#45). Also fixes Travis CI testing, which broke unexpectedly.

@gaurav gaurav marked this pull request as ready for review May 7, 2020 05:39
@gaurav gaurav requested a review from hlapp May 7, 2020 05:40
Copy link
Member

@hlapp hlapp left a comment

Choose a reason for hiding this comment

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

I might be missing something but it doesn't seem like you're testing non-resolving phylorefs, nor malformed queries raising the expected error.

bin/resolve.js Outdated Show resolved Hide resolved
bin/resolve.js Outdated Show resolved Hide resolved
bin/resolve.js Outdated Show resolved Hide resolved
@gaurav
Copy link
Member Author

gaurav commented Oct 21, 2020

I might be missing something but it doesn't seem like you're testing non-resolving phylorefs, nor malformed queries raising the expected error.

Thanks for pointing this out, @hlapp! I've added four additional tests:

  • A test for phylorefs with a specifier that is present in the Open Tree Taxonomy, but not on the synthetic tree, such as Diplocynodon ratelii, which produces a 400 error from the Open Tree of Life.
  • A test for phylorefs with a single internal specifier (which cannot be resolved).
  • A test for phylorefs that contain specifiers that are not present in the Open Tree Taxonomy (which was not being handled correctly).

Are there any other cases we should test for?

@gaurav gaurav requested a review from hlapp October 21, 2020 03:46
@hlapp
Copy link
Member

hlapp commented Oct 21, 2020

@gaurav as commented in a previous PR, can I please ask you to rebase first for merge ability so that you are asking me to review what would actually get merged. Right now as a reviewer I can't know that.

@gaurav
Copy link
Member Author

gaurav commented Oct 22, 2020

Sorry, Github said that "This branch has no conflicts with the base branch", which I assumed meant that it was mergeable. I've now run git rebase master and force-updated this PR so that all the changes from master are present here as well.

Copy link
Member

@hlapp hlapp left a comment

Choose a reason for hiding this comment

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

I'm approving this though please consider the one in-line comment I left.

test/examples/otl-resolution-errors.json Outdated Show resolved Hide resolved
@gaurav
Copy link
Member Author

gaurav commented Nov 9, 2020

Github/Travis CI integration seems to be acting up right now, but the final commit (f03a1f6) has passed testing on TravisCI at https://travis-ci.org/github/phyloref/phyx.js/builds/742347928. I'm going to go ahead and merge it.

@gaurav gaurav merged commit 9007075 into master Nov 9, 2020
@gaurav gaurav deleted the open-tree-cli branch November 9, 2020 04:29
@gaurav gaurav restored the open-tree-cli branch November 19, 2020 19:54
@gaurav gaurav deleted the open-tree-cli branch November 19, 2020 19:54
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.

2 participants