Skip to content
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

diamond: Add diamond exercise #408

Merged
merged 1 commit into from
Oct 25, 2016

Conversation

ferhatelmas
Copy link
Contributor

No description provided.

@petertseng
Copy link
Member

petertseng commented Oct 18, 2016

Thanks.

We should discuss whether the tests should be example-based/value-based (as they are currently written) or property-based as the README and the source blog post have it the source blog post have it

@petertseng
Copy link
Member

Asking at exercism/problem-specifications#191 tells me that it is preferred that we test specific properties of the solutions, rather than the exact values - to show a different way of testing, since all other problems are going to be using example-based tests. Do you think it will be possible to implement the property-based tests in this language?

Obviously the example.go can stay the same!

@ferhatelmas
Copy link
Contributor Author

@petertseng I hadn't know about property-based testing before but after some reading about it, seems superior. Thanks for pushing in this direction!

@petertseng
Copy link
Member

Nicely done, I'm glad this worked out! In fact I will have to explore testing/quick for myself soon, see how it works! I also realize I meant to link http://blog.ploeh.dk/2015/01/10/diamond-kata-with-fscheck/ instead of http://claysnow.co.uk/recycling-tests-in-tdd/ but it seems like we have come up with something good here despite my mistake. Okay, I'll take a look.

@@ -76,7 +76,8 @@
"connect",
"ledger",
"poker",
"variable-length-quantity"
"variable-length-quantity",
"diamond"
Copy link
Member

Choose a reason for hiding this comment

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

any thoughts on moving this up earlier to get the exposure a bit earlier? I'm only blindly guessing at the difficulty, but it seems once someone understands how to work with the tests it the task itself can be less complex than, say, connect and poker. What about moving it before, say, react?

We don't have to worry too much about getting the order exactly right though - we'll have a better idea of the order when #345 is complete. But let's try to make a guess now and we can adjustg later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, put before react.

@petertseng
Copy link
Member

Everything looks good and it looks like the tests chosen were exactly the ones in the README. So I think after we decide on a location to place this problem, this will be ready to merge.


If you have time to discuss some things not related to this PR in particular, but tangentially related to it:

I haven't solved this exercise myself yet in any other language, and I think we are one of the early tracks to do this property-based testing (others are using example-based except C# and F#)

Did you feel the sequence of tests in the README was logical? Does it allow incremental progress toward a solution? Was there any test that will always pass with no additional effort if the previous ones have been taken care of?

I do know that https://github.com/exercism/xfsharp/blob/master/exercises/diamond/DiamondTest.fs and https://github.com/exercism/xcsharp/blob/master/exercises/diamond/DiamondTest.cs chose a slightly different order from us, but different things may make sense for different tracks.

Any feedback you have can help us improve the README.

@petertseng
Copy link
Member

Here's a question, let me know if I missed something - is there something that makes sure that the diamond extends to the correct letter? For example, that if I ask for Gen('B') I don't get the triangle for C?

@petertseng
Copy link
Member

Ah, indeed, if I make the function always return "A\n" as long as the byte is in-range, all the tests do pass. So I guess we should add some test that checks that we go to the right letters.

@ferhatelmas
Copy link
Contributor Author

ferhatelmas commented Oct 19, 2016

@petertseng Thanks for catching this mistake. I changed TestDiamondIsSquare to reflect it and also added skips to make it more incremental as other tracks.

@petertseng
Copy link
Member

Thanks! Can you change all the "Remore" to "Remove"?

Too bad about the Skip being needed, I wish we could take the same approach as the other exercises, but Fatalf seems to only stop the test it's used on. So it loiked like the skip is necessary. Is there any concern about students not knowing that they have to remove the skip? For example, whe simply running go test with skips, all we can see is:

PASS
ok      _/home/pt/src/xgo/exercises/diamond 0.003s

I wish there were an indication shown here that there have some tests that were skipped. using go test -v would do it, but what about any other ways?

If we can't make things work out, I suppose we could switch to using Fatal... if we mashed all the current tests into one function, though I'm not sure it's what we want to do. Any ideas you have to address this would be helpful.

@ferhatelmas
Copy link
Contributor Author

I removed skips at all because test-without-stubs after fresh get will give errors if we use fatal. I think, this isn't what we want.

Also, removed main function since it was actually unnecessary.

@ferhatelmas
Copy link
Contributor Author

@petertseng Let me know what else is needed. Otherwise, I will rebase and squash commits for merge.

@kytrinyx
Copy link
Member

I think we're ready to get this merged! Would you rebase and squash?

 * property-based tests
@ferhatelmas
Copy link
Contributor Author

ferhatelmas commented Oct 23, 2016

@kytrinyx Rebased and squashed. Good to go!

@petertseng petertseng merged commit 961e270 into exercism:master Oct 25, 2016
@petertseng
Copy link
Member

Thanks! Nicely done!

@ferhatelmas ferhatelmas deleted the add-diamond-exercise branch November 24, 2016 22:42
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