-
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 #2057
base: bellsofaba
Are you sure you want to change the base?
Implement _check #2057
Conversation
Thank you for the submission, @bellsofaba! 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, @bellsofaba!
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.
Your code style completely follows the style guide. Nice job!
There are 5 tests still pending. In your test file, change describe.skip
to describe
and ensure they pass.
Couldn't help noticing most of the few PR also have the 5 pending tests. Are these negligible and are something directly related to the setup of the codebase or is that something each and everyone of us has to make changes to. Tried having a go at it but got stuck pretty bad and couldn't get passed the pending test. |
Hey @bellsofaba, everything so far has been an automated response. It's not equipped to handle questions such as these. Hopefully I can help you out though. To get this PR merged, you'll have to pass all of the tests, even the pending ones. To change the pending tests to active, you have to change Then run I hope this helps |
Hopefully, this goes to my branch.