-
Notifications
You must be signed in to change notification settings - Fork 35k
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
feat(totalIntegers): create exercise #443
base: main
Are you sure you want to change the base?
feat(totalIntegers): create exercise #443
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.
General but important comment to consider for this one - I'll let you have a think about how you want to approach this.
- No need to reinvent the wheel for isInteger
- You only test nested arrays but what if there are objects within the array(s) that contain integer values?
e.g.
[4, 6, { a: 1, b: { a: 10, b: 11 } }, 9]
Didn't know that method exists, how convenient! Good point about the objects with numbers, that certainly makes the challenge more interesting |
|
Wait whaaaat xD |
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.
Haven't had time to assess the tests but at least in terms of how the solution is written, here are some suggestions.
Co-authored-by: MaoShizhong <[email protected]>
Co-authored-by: MaoShizhong <[email protected]>
Co-authored-by: MaoShizhong <[email protected]>
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.
2 more general comments:
- Don't forget to add
.skip
to all but the 1st test in the suite. - I know the earlier exercises use a traditional function expression but I think it may be sensible to have these new exercises use arrow functions, and also for the non-solution template. The older exercises can always be updated another time. @JoshDevHub, thoughts on having the exercises use arrow functions instead of traditional func expressions?
For the first point, I am pretty sure that the Also I agree that it would be nice to have them all be arrow functions however I think consistency is important and it would be better to update them in a different PR. Also considering that the exercise generator tool would need to be updated too it would certainly have to all be in a separate PR. |
Ah yes, the solution test suite doesn't. As long as the skips get added when you copy over for the non-solution test suite, then all is good 👍 Also fair point on the exercise generator tool - let's stick to trad. function expressions then. |
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.
Left some thoughts. Let me know if you have any questions.
@NikitaRevenco Any issues with the above comments from Josh? |
… of the function was not being passed to any function calls
Co-authored-by: Josh Smith <[email protected]>
Ok, went over all the comments just now and added the suggestions |
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.
After the comment below, I'm happy to approve once the tests and appropriate skips have been copied over to the other spec file 🔥
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.
Other than Mao's one live comment, I think everything looks good with this.
Because
It was decided to add new recursion exercises
Previous
This PR
Issue
Related to #27265
Additional Information
Pull Request Requirements
location of change: brief description of change
format, e.g.01_helloWorld: Update test cases
Because
section summarizes the reason for this PRThis PR
section has a bullet point list describing the changes in this PRIssue
section/solutions
folder