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

fix: use latest GO api changes #542

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

fix: use latest GO api changes #542

wants to merge 2 commits into from

Conversation

jsstevenson
Copy link
Contributor

Two weird changes seem to have happened with the GO API:

  1. it doesn't like the quotes in the URL anymore
  2. if pages are exhausted, it gives a 404 rather than returning an empty array

this PR adds fixes for both

Copy link
Collaborator

@acoffman acoffman left a comment

Choose a reason for hiding this comment

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

One small question, otherwise looks good!

genes.each do |gene|
create_gene_claim_for_entry(gene, category) if gene['taxon_label'] == 'Homo sapiens'
end
break if genes.count < rows
Copy link
Collaborator

@acoffman acoffman Nov 27, 2024

Choose a reason for hiding this comment

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

Does this fail if the number of rows on the last page happens to be the same as the page size you and run into the raise StandardError in the api client on the subsequent request?

If so, maybe you want to handle that separately.

You can directly check the http response code for 404 (as it is currently doing with 200), or you could match on the response type like this example here if you find that to be cleaner. (Note it will not be redirection as the subclass but NotFound). I could be misunderstanding what's going on though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, totally. I was being lazy.

it seems like the GO API returns the same response payload for a bunch of different classes of errors:

{"detail":"Item with ID GO:0008233 not found"}

You get a variation of this ^ if you request a nonexistent entity, but also if you make a request where "start" is beyond the number of records. I was trying to avoid catching 404s because, like, those are nice to know about for other reasons, but maybe we can condition this such that we raise the exception if the initial request with start=0 returns a 404, but we just break the loop once we're starting from a higher number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's also a good chance that this is a recent bug that will get fixed, but the issue tracker for the GO site has like 50 open PRs and hundreds of open issues, so... idk

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