-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Implement _check #2258
Implement _check #2258
Conversation
Thank you for the submission, @javanpang! I'll review your code shortly, hang tight. |
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 is a great attempt, @javanpang!
I would like to request a few changes before merging your work. Please review my comments below and make the appropriate changes to your code.
After you update your code locally, follow the instructions to save your changes locally and push your changes to your fork.
When you push your changes to your fork, I'll come back for another review.
There are 2 style guide violations in your contribution. I've marked them with inline comments for your convenience.
Please revisit your code and follow the style guide best practices.
Hint: You might be able to fix some issues automatically by running npm run lint -- --fix
All the tests are passing. Nice job!
src/calculator.js
Outdated
// HINT: you can invoke this function with exports._check() | ||
exports._check = (a, b) => { | ||
if (typeof a !== 'number') { | ||
throw new TypeError(`${a} is not a number`) |
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.
Missing semicolon.
(learn more)
src/calculator.js
Outdated
throw new TypeError(`${a} is not a number`) | ||
} | ||
if (typeof b !== 'number') { | ||
throw new TypeError(`${b} is not a number`) |
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.
Missing semicolon.
(learn more)
Thanks for the changes, @javanpang. I'm reviewing them now. |
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.
LGTM 👍
Thanks for your contribution @javanpang!
Your code style completely follows the style guide. Nice job!
All the tests are passing. Nice job!
🎉 You did it! 🎉You're an open source contributor now! Whether this was your first pull request, or you’re just looking for new ways to contribute, I hope you’re inspired to take action. Don't forget to say thanks when a maintainer puts effort into helping you, even if a contribution doesn't get accepted. Open source is made by people like you: one issue, pull request, comment, and +1 at a time. What's next?Find your next project:
Learn from other great community members:
Elevate your Git game:
Questions? Comments? Concerns?I'm always open to feedback. If you had a good time with the exercise, or found some room for improvement, please email me. Want to start over? Just delete your fork. |
Solved issue #1 by optimising export._check() function in calculator.js to delete duplicate code and pass all tests.