-
Notifications
You must be signed in to change notification settings - Fork 3
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
Improve behavior on errors when importing a dataset #74
base: dev
Are you sure you want to change the base?
Conversation
const data = loadEpidata(title, res, columns, columnRenamings, { _endpoint: endpoint, ...params }); | ||
if (data.datasets.length == 0) { | ||
try { | ||
const data = loadEpidata(title, res, columns, columnRenamings, { _endpoint: endpoint, ...params }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we put the try
block around just this loadEpidata()
line? if the other lines are throwing exceptions, we should handle them in a different way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is already the case? The try block is around three lines:
const data = loadEpidata(...)
if (data.datasets.length == 0) {...}
return data;
And the last two can't really error out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats what im saying -- the try
scope should contain only the line(s) where we anticipate a possible exception
src/api/EpiData.ts
Outdated
return UIkit.modal | ||
.alert( | ||
` | ||
<div class="uk-alert uk-alert-error"> | ||
<a href="${url.href}">API Link</a> returned no data. | ||
Failed to fetch API data from <a href="${url.href}">API Link</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add more information to this, like the returned status code at the very least? when a user runs into the rate limiting, this will be helpful. in fact, when a 429 is served via the API, it returns this html in the body, which would be even more useful to a user:
<!doctype html>
<html lang=en>
<title>429 Too Many Requests</title>
<h1>Too Many Requests</h1>
<p>Rate limit exceeded for anonymous queries. To remove this limit, register a free API key at https://api.delphi.cmu.edu/epidata/admin/registration_form</p>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also fixed - it should now include the error code if it's not inside the JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isnt working for me when it encounters a 429 from making too many requests... it still just says "Failed to fetch API data from API Link."
Closes #45.
In the Import Dataset dialogue, displays the errors from the EpiData API to the user if one is present: