-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 #128
Implement _check #128
Conversation
Thank you for the submission, @caulifla! 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.
Great attempt @caulifla, you're almost there!
I've marked a few style guide violations with inline comments. Please review the comments below and make the appropriate changes. Remember, you can check your code's style locally with npm run lint
.
After you update your code, follow the instructions to save your changes locally for the files you have updated. When you send your changes to your fork, I'll come back for another review.
src/calculator.js
Outdated
}; | ||
|
||
exports.add = (x, y) => { | ||
exports._check(x, y); |
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.
Trailing spaces not allowed.
See http://eslint.org/docs/rules/no-trailing-spaces for details about this rule
src/calculator.js
Outdated
return x / y; | ||
}; | ||
|
||
module.exports = exports; | ||
module.exports = exports; |
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.
Newline required at end of file but not found.
See http://eslint.org/docs/rules/eol-last for details about this rule
src/calculator.test.js
Outdated
@@ -134,4 +134,4 @@ describe('divide', () => { | |||
it('should divide one positive number and one negative number', () => { | |||
expect(calculator.divide(84, -2)).to.equal(-42); | |||
}); | |||
}); | |||
}); |
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.
Newline required at end of file but not found.
See http://eslint.org/docs/rules/eol-last for details about this rule
I'm reviewing your changes now, @caulifla. Let's get this merged 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.
All of the tests are passing and the code is clean.
This is a huge improvment for our codebase's maintainability. The team is going to be stoked to see your contribution when they get back from vacation.
Well done, @caulifla 👏
🎉 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. This branch will be automatically deleted after one week with no merged pull requests |
Hello again, @caulifla! I want to congratulate you again for completing this challenge. Hopefully you've started to get out there and apply your knowledge to another project. Since you haven't pushed a commit in a while, I'm going to delete your branch in order to make room for others actively particiating. You'll still have access to your fork, but you won't be able to submit anymore pull requests to this branch. Don't worry though! If you decide that you do want a new branch, you can delete your fork and re-fork the original project. Happy Hacking! |
Fixed issue #1