Skip to content

Add descriptions for the raindrops tests. #450

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

Merged
merged 2 commits into from
Nov 23, 2016
Merged

Conversation

stevejb71
Copy link
Contributor

Unlike the other canonical-data I have seen, the tests have no description.
I'm writing a test generator for the xocaml stream - it fails due to this.

Copy link

@dvberkel dvberkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a solid improvement. I have a single question, but certainly no show stopper.

"number" : 10,
"expected": "Plang"
},
{
"description" : "the sound for 14 is Plong",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rather like the cases for 9, and 10, where there is an explanation for the sound in the description. It is missing here, is this by design?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's just a mistake. I'll correct it when I get back to my pc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the description for 14.

@petertseng
Copy link
Member

I apologise for breaking the upcoming generator - this was my fault in #314. It seems in light of #336 it is advantageous to keep things consistent. The good part: since the new descriptions are not just the string representation of the number, then it actually makes sense to have te descriptions.

"number" : 6,
"expected": "Pling"
},
{
"description" : "the sound for 8 is 8",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to include the reasoning for #370 in here? In particular, that 2^3 is not "Pling" since the 3 is the power, rather than the base?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a common mistake in implemention is this exercise, then why not.

Something like "2 to the power 3 does not make a raindrop sound as 3 is the power not the base"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That proposed description seems good.

Caveat on the strength of my opinion: Whether it qualifies for common isn't known first-hand to me as I haven't done this exercise in any language. A particular way of doing it in Ruby (exercism/ruby#434) first caused this case to be added.

But I figure... now that it's added, might as well give it the description to suit it, and I think this description does suit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the description for 8.

Add better explanations for 8 and 14.
@dvberkel
Copy link

Changes look good to me

@stevejb71 stevejb71 merged commit 7d1e349 into master Nov 23, 2016
@stevejb71 stevejb71 deleted the StandardizeRaindrops branch November 23, 2016 14:16
petertseng added a commit to exercism/rust that referenced this pull request May 30, 2017
Added in exercism/problem-specifications#370

Although I doubt anyone will make the same mistake in this language, I
don't care enough to declare that we should elide these cases.

Note that even though Rust now has the same cases as 1.0.0, we don't
have descriptions like in exercism/problem-specifications#450.
I don't find them terribly necessary, but I suppose they can be added if
they seem good.

We would likely no longer be able to have the tests on one line in that
case, since some descriptions are rather long.
emcoding pushed a commit that referenced this pull request Nov 19, 2018
Add test for 10 card; add test comparing 5-high straight to other str…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants