-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add kindergarten-garden
exercise
#294
Conversation
Hello 👋 Thanks for your PR. This repo does not currently have dedicated maintainers. Our cross-track maintainers team will attempt to review and merge your PR, but it will likely take longer for your PR to be reviewed. If you enjoy contributing to Exercism and have a track-record of doing so successfully, you might like to become an Exercism maintainer for this track. Please feel free to ask any questions, or chat to us about anything to do with this PR or the reviewing process on the Exercism forum. (cc @exercism/cross-track-maintainers) |
|
||
#[test] | ||
#[ignore] | ||
fn second_students_garden() { |
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 test name is pretty similar to the one before it. To avoid confusion, I'd consider perhaps fn multiple_students_bobs_garden () {
. The next test can then be fn multiple_students_charlies_garden () {
.
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 usually name tests after canonical data's description. For these two, the descriptions were respectively "second student's garden"
and "third student's garden"
, see canonical-data.json
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.
Seems like an enum to represent the plants might be appropriate here.
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.
Good suggestion, I extracted students into an enum too, see 6eb635e
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.
Meant to approve. My two comments are just suggestions.
@BNAndras let me know if you like these new enum changes, and we can merge |
Closes #194