-
Notifications
You must be signed in to change notification settings - Fork 17
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
Feras AL Demashki w1-usingAPIs-assignment #53
base: main
Are you sure you want to change the base?
Feras AL Demashki w1-usingAPIs-assignment #53
Conversation
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.
Hi @Feras-ALdemashki, overall well done! But there are some minor issues that I would like you to look at before I can approve your PR.
reject(Error("You didn't pass in a first name!")); | ||
} | ||
}, 1000); | ||
}); | ||
}; | ||
|
||
function main() { |
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.
When I run your code (using npm start
rather than npm test
) nothing is displayed any more in the console. Since you are now using promises you need to modify the main()
function too.
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 have changed the main() function and checked the npm start its working now .
@@ -1,3 +1,5 @@ | |||
import { error } from '@actions/core'; |
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 assume this got inserted by VSCode by accident. Better get rid of it.
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 don't know what is this line is . now i removed it
} | ||
|
||
// ! Do not change or remove the code below | ||
if (process.env.NODE_ENV !== 'test') { | ||
main(); | ||
} | ||
|
||
// TODO Replace this comment by your explanation that was asked for in the assignment description. | ||
// the previous problem is no longer exist. | ||
// now we return the promise so the rolling will stop after the promise is resolved or rejected |
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.
Not quite. The rolling does not stop after a reject. The die continues to roll until it has completed its (randomly) assigned rolls. Can you improve your explanation?
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 have changed the explanation .
@@ -43,4 +42,4 @@ if (process.env.NODE_ENV !== 'test') { | |||
main(); | |||
} | |||
|
|||
// TODO Replace this comment by your explanation that was asked for in the assignment description. | |||
// now we have 5 different promises, and each one is running as a different task in an async 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.
👍
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.
Thanks, @Feras-ALdemashki. I can now approve your PR. Good luck with the week 2 assignment.
getAnonName('John', console.log); | ||
getAnonName('John') | ||
.then((response) => console.log(response)) | ||
.catch((error) => console.error(error.message)); |
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.
👍
@@ -47,4 +47,4 @@ if (process.env.NODE_ENV !== 'test') { | |||
} | |||
|
|||
// the previous problem is no longer exist. | |||
// now we return the promise so the rolling will stop after the promise is resolved or rejected | |||
// because the promise only settle once , any other resolves or rejects will be ignored |
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.
Yep
No description provided.