erlang
erlang copied to clipboard
exercism
implement exercise binary-search-tree
Hello @voila!
As we have canonical data available in JSON format, could you please open a PR at https://github.com/exercism/erlang-tests-generator, adding the testgenerator and then pushing the generated tests on this branch?
I totally understand if you prefer to have the manually implemented tests merged as they are, but then you have to wait up to the end of the month before I can do a proper review (of course I try to as fast as possible).
Hello @voila!
Do you want me to review as is or do you want to give the test-generator a try?
@voila you have been pretty active with your other PR.
Please show some activity here as well and answer at least the open question, as well as fixing merge conflicts and obviously wrong comment in the test file.
I'm feeling forced to close this in ~2 days otherwise.
Do you want me to review as is or do you want to give the test-generator a try?
Can you review as is, please ?
Can you review as is, please ?
I'll do first comments through the day, though I'll be on a business trip for a few days then. Not sure if I will be able to continue review before Thursday, so you can take your time for doing the actual changes then.
There are some tests in the suite like ..._can_sort_..., based on this set of tests in the canonical data.
In the canonical data, the property is called sortedData, but your implementation uses to_list. I think this may be misleading to students, as nothing really tells them that the result should be a sorted list. Your tests however, fix the order of the elements in the expected result.
You should either align the function name with the canonical data (which is what I would suggest/prefer), or accept any order of items (which would go against what is in the canonical data and thereby make implementing a test generator more difficult, so I don't recommend it).
@NobbZ thoughts?
As we have canonical data, I prefer to be as close as possible, and of course I also expect the corresponding version to be set in *.app.src and as a comment in the test suite.
I'll do a more in depth review later.
Looks good to me, @juhlig can you please take another look and merge, I am on mobile only and am a bit frightened that I've overlooked something.
Sorry, I missed you mentioning my name ;) I think this one is good to go, as far as I can see.