Test for Nothing case in Diamond exercise

[chiroptical]

When I solved this exercise, my first solution didn't handle the Nothing case. This PR simply adds a test to check that diamond '1' returns Nothing to hint to the user their solution is incomplete. Any comments are appreciated.

[sshine]

Hi @barrymoo, and thanks for contributing!

TL;DR:

• I am reluctant to include this negative test case. I'd like to hear what @petertseng thinks.
• If you'd like to instead write property-based tests for this or another exercise, this could be a learning experience. I did this in #735 for another exercise.

The long version:

I think your choice of '1' fits nicely with the existing equivalence classes, since we leave lowercase letters and non-ASCII letters unspecified as the text does. But I wonder if some solutions that use chr and ord naively may give way to different edge cases above and below the ['A'..'Z'] range.

Negative cases were intentionally omitted in e102159. But for another Maybe exercise, Grains, negative cases were included in a3861fd. You could argue for both the similarity and the difference between the two. When you look at exercism/problem-specifications#556 which discusses the canonical test suite coverage, there is surprisingly no discussion of handling negative cases at all.

Either there is some implicit argument nobody bothers to mention, or there isn't.

I'll try and list some considerations:

• For the sake of coverage, having a negative case could be a good thing.
• For the sake of compliance with Diamond's canonical test suite, we must decide if we should (1) keep the tests as-is, (2) deviate from the canonical tests, (3) propose that the canonical tests are also extended, (4) replace the current tests with property-based tests as suggested in the canonical test suite and as implemented on e.g. Diamond on exercism/go, or (5) something else entirely.

The Diamond exercise was changed to use Maybe in #653 after a lengthy discussion in #626.

In the design process, it was proposed (comment) that:

I don't think we should specify how the functions should behave for each possible input as a rule, because that would limit the diversity of solutions. Sometimes people surprise us with interesting solutions that wouldn't pass a unnecessarily strict test suite.

The main problem here is that we don't allow the diligent student to signal invalid inputs in a idiomatic way, so I think we should change the signature. But I don't think we need to include additional tests to check if the solution correctly handles those cases.

In this particular exercise, an alternative solution without Maybe could be this one (comment):

data Letter = A | B | C | D | E | F | G | H | I | J | K | L | M
            | N | O | P | Q | R | S | T | U | V | W | X | Y | Z
            deriving ...

diamond :: Letter -> [String]
diamond = ...

If I understand this argument, we should seek to minimize the edit distance in the test file to encourage such alternative, creative solutions. We could, however, still have a negative test on its own line ready to be commented out that doesn't affect multiple Justs spread out in the file.

An alternative is to add -Werror=incomplete-patterns to package.yaml so that partial solutions will not compile.

If one instead were to add property-based testing, a positive test generator could pick among ['A'..'Z'] and a negative test generator could pick an arbitrary not . isLetter character and still comply with the fuzzy exercise text. In #600 property-based testing for Diamond in Haskell is discussed to great length.

I would recommend that we don't try and push a negative test to the canonical tests right now because the problem-specifications repository is temporarily only accepting bug-fixes.

[chiroptical]

Thanks for the great comments!

If you'd like to instead write property-based tests for this or another exercise, this could be a learning experience. I did this in #735 for another exercise.

I have experience with QuickCheck. If you think this is a better idea I am willing to give it a shot.

I would recommend that we don't try and push a negative test to the canonical tests right now because the problem-specifications repository is temporarily only accepting bug-fixes.

Just so I understand, this statement is recommending implementing PBTs yes?

If you would like to close this, feel free. I can submit a different PR with the PBTs.

[sshine]

Yes, exactly! There are some thoughts on #600 to get started.