Skip to content

triangle: Rewrite test do use hspec with fail-fast.#237

Merged
rbasso merged 1 commit intoexercism:hspec-fail-fastfrom
rbasso:hspec-triangle
Jul 31, 2016
Merged

triangle: Rewrite test do use hspec with fail-fast.#237
rbasso merged 1 commit intoexercism:hspec-fail-fastfrom
rbasso:hspec-triangle

Conversation

@rbasso
Copy link
Copy Markdown
Contributor

@rbasso rbasso commented Jul 26, 2016

Related to #211.

@petertseng
Copy link
Copy Markdown
Member

Everything seems fine, 👍

I notice these are in a much more abbreviated style. I feel like some other test files could be written in this abbreviated style as well, e.g. roman-numerals where there should be no chance of confusion between input/expected

@petertseng
Copy link
Copy Markdown
Member

This is not related to hunit->hspec, but exercism/problem-specifications#298 got merged so at some point soon we might rethink the (1, 2, 1, Illogical) case (triangle inequality implies it should be isosceles), but maybe some cross-track decision needs to be made there. We have flip-flopped on that decision before.

@petertseng
Copy link
Copy Markdown
Member

Also you wrote "do use" in commit message rather than "to use"

@rbasso
Copy link
Copy Markdown
Contributor Author

rbasso commented Jul 30, 2016

I'm still working on these PR, to make them more consistent with what was done on the previous ones exercises:

  • robot-name
  • meetup
  • beer-song
  • triangle
  • scrabble-score
  • roman-numerals
  • binary

I notice these are in a much more abbreviated style. I feel like some other test files could be written in this abbreviated style as well, e.g. roman-numerals where there should be no chance of confusion between input/expected.

At first, I used tuples instead of data Case = ... in most of the exercises. Some of them are simpler and very readable that way, but others are harder to understand and the huge tuples are messy.

To keep consistency and ease maintenance, I rewrote all the previous exercises to mimic the structure of the JSON file using a custom data type. This is certainly more verbose than using tuples, but is almost identical to the reference files and scales betters to more complex exercises.

My original plan was to rewrite those remaining PRs to follow the same pattern.

The decision is mostly about "homogeneity vs simplicity". I completely agree that triangle and roman-numerals look better using tuples, but I was ready to sacrifice those advantages to keep a standard implementation for all the exercises that have a JSON file.

I should have discussed that before, sorry. That was something that naturally evolved as I was porting the exercises to hspec and matching the tests the JSON files, and I seemed the best solution to me.

If you prefer I can use tuples on the open PRs and I'll merge new PRs to change the previous exercises in branch hspec-fail-fast.

Edit: For exercises like triangle that have no JSON file, I see no big advantage in using a custom data type, unless the tuple-solution is too ugly.

@petertseng
Copy link
Copy Markdown
Member

I think ultimately there are problems that can use either a tuple or a record. We see that triangle is one of them. For those, I don't think I would complain either way.

And there are problems that probably look better with a record. I'm sure you came across some examples. I am not sure if I can find one off the top of my head, maybe all-your-base?

I was ready to sacrifice those advantages to keep a standard implementation for all the exercises that have a JSON file.

This could be a good advantage. Currently the knowledge of how standard are the implementations is in your hands since you've done all of the hunit->hspec conversions to date. The corollary to that is you may know best which choices will cause less work for the remaining conversions. If there ever comes a time when it makes sense to put any tools for generating hspec cases from x-common json files into this repo, others may have a better idea of what choice to make.

Currently I do not see the need to insist on any changes to any currently hspecced exercise, but we can discuss if you think you would like to change it after all.

@rbasso
Copy link
Copy Markdown
Contributor Author

rbasso commented Jul 30, 2016

This is not related to hunit->hspec, but exercism/problem-specifications#298 got merged so at some point soon we might rethink the (1, 2, 1, Illogical) case (triangle inequality implies it should be isosceles), but maybe some cross-track decision needs to be made there. We have flip-flopped on that decision before.

Considering that this would break the existing solutions, I prefer to wait a little until we see where the consensus goes. A json file would be great, but my focus now is on rewriting the tests to use hspec, and it'll be a while until I have time to take a look at that.

@rbasso rbasso merged commit 0352918 into exercism:hspec-fail-fast Jul 31, 2016
@rbasso rbasso deleted the hspec-triangle branch July 31, 2016 06:57
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.

2 participants