-
Notifications
You must be signed in to change notification settings - Fork 1
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: Updates api ip address & fix loop #15
base: main
Are you sure you want to change the base?
Conversation
const data = await axios.get( | ||
'http://141.94.127.211:8000/get_unlabeled_random_event', |
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 can have a look later but here we are doing something that is not great : we are awaiting for request i
to finish before triggering request i+1
.
A better approach would be to run all requests in parallel with Promise.allSettled
. Promise.allSettled
is better in this case than Promise.all
because it won't throw an error if one of the call fails.
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.
Overall the best approach would be to have a route that returns a list of events, so that we don't need to trigger n
calls to get n
events.
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 am kind of react/tsx noob to implement Promise.all/allSettled but you are right, we want to avoid error if one of the call fails !
Yes, i agree -> a route returning a list of events (with a param n
and a default value !) -> I am taking care to have this route soon !
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 think we shall implement this dedicated route later and go on with the suggested approche in this PR because at least it fixes some previous issues (even if it is not optimal)
I've taken your feedback into account. Concerning the one on the need for a new road to improve traffic flow, I suggest that we deal with this later (in another small PR). Can you please review again and maybe approve the PR ? :) happy to discuss it ! |
Hi there,
This PR introduces the following modifications :
Updates API ip url -> the old server was not working anymore
Modifies the loop to get as many images as wanted in the loop -> With the previous code, I always ended up with just one event (and the magnificent koala) to categorise. I have the feeling that there was a return problem in the loop ("return image"s arriving too early), but I'm not at all familiar with typescript !
Happy to discuss it :)