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: Updates api ip address & fix loop #15

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

Conversation

fe51
Copy link
Member

@fe51 fe51 commented Dec 12, 2024

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 :)

@@ -10,7 +10,7 @@ export const useImages = () => {
for (let index = 0; index < 50; index++) {
try {
const data = await axios.get(
'http://141.94.127.211:8000/get_unlabeled_random_event',
'http://57.128.107.129:8000/get_unlabeled_random_event',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can extract http://57.128.107.129:8000 in a top level const so we don't forget to update both strings when we change the ip of the server

Comment on lines -22 to -30
const koalaImage =
'https://images.unsplash.com/photo-1459262838948-3e2de6c1ec80?q=80&w=1080&auto=format&fit=crop&ixlib=rb-4.0.3&ixid=M3wxMjA3fDB8MHxwaG90by1wYWdlfHx8fGVufDB8fHx8fA%3D%3D';

images = [
...images.slice(0, 10),
{ gif: koalaImage, img_list: [], id: 0 },
...images.slice(10),
];
return images;
Copy link
Collaborator

@antoine-cottineau antoine-cottineau Dec 26, 2024

Choose a reason for hiding this comment

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

Good fix! I don't think it's a good idea to add an easter egg that adds that much complexity, especially for a POC

Comment on lines 12 to -13
const data = await axios.get(
'http://141.94.127.211:8000/get_unlabeled_random_event',
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Member Author

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 !

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