resistor-color-duo: Change to value :: (Color, Color) -> Int

[sshine]

TL;DR:

• I don't think we should be teaching people to write functions like this.

• The less controversial action would be to reject this PR in favor of one
  where either

   value :: [Color] -> Maybe Int
   value :: [Color] -> Either String Int
   value :: [Color] -> Either ColorError Int
  

Quick chronological recap of PRs that provide necessary context:

• 2019-03-08: exercise created in resistor-colors: add new exercise problem-specifications#1466
• 2019-03-12: exercise ported to Haskell track in Implement Resistor Colors exercise #808
• 2019-08-17: third-band test case and formulation "brown-green-violet should
  return 15 too, ignoring the third color." + one other similar formulation
  added to README.md in resistor-color-duo: add test case to ignore additional colors problem-specifications#1569.
• 2019-10-01: third-band test case added to Haskell track in resistor-color-duo 2.1.0.2: Ignore bands beyond the second #849.

When this exercise was ported to Haskell track, it was decided to not address
error handling in this exercise via Maybe Int or similar, which otherwise
would be necessary since value is not defined for [Color] of length 0 or 1.

The alternative proposed here, to limit the input to exactly two bands, does
not appear to be mentioned.

In the meantime, the third-band test case (and formulations) have made value
explicitly more ambiguous: Unlike 0 or 1 bands, which is undefined, the third
band should be ignored, and >3 bands are undefined. Presumably they should be
ignored as well, but following this logic, should 0 or 1 bands also be ignored?
Why, exactly, are we creating a function so vulnerable to wrong input and not
let it handle validation gracefully?

README.md explicitly asks to disregard a third color in two places. Given this
proposal, those messages become meaningless; you can only specify two bands.

I can think of these ways to address the deficiency that this change causes:

• Remove the lines from README.md, breaking a track invariant and CI check.
• Apply a track-specific hint saying how this exercise is adapted to Haskell
  track in this way.
• Don't change the description; confuse students. ("What third band?!")
• Reject this PR in favor of another solution, as highlighted above.
• Something else entirely.

The Read instance is removed from the stub as it isn't necessary.

This changes the version from 2.1.0.2 to 0.1.0.3, because the three-band test
case is removed, which makes the test suite no longer reflect canonical tests.

[sshine]

@petertseng: Maybe this was why you didn't do #849 for a while.

@barrymoo: I saw you solve this exercise and come to the same conclusion as this PR.

If this PR is rejected, you are welcome to do one of the proposed less controversial alternatives.

[petertseng]

Upon review, it's indeed better to have the type system help this function be called in the correct way.

Regarding the README: I think the way of least resistance is to add a track-specific hint.

I wish it were easier to have README generation apply a subtractive change, so that we could just remove the strings "even if the input is more than two colors" and "brown-green-violet should return 15 too, ignoring the third color." from the README. But to be able to put that inconfiglet it would require more design. Other option is if we wrote and maintained our own README generator and used it instead.

We also may place a description.md in the .meta directory but I don't like to burden us with maintaining one which we'd then possibly want to keep up-to-date with any changes in problem-specifications. We'd have no way of knowing when our custom description.md (which is based on problem-specifications's one, but not exactly the same) is out of date.

I also have a comment regarding the version.

I am likely to approve once the README is done but I figured you would probably want to ask for a review on that text anyway.

[petertseng]

I would actually be okay with 2.1.0.3, and furthermore I think 2.1.0.3 is a better choice.

The only specification we have in https://github.com/exercism/haskell/blob/master/README.md#exercise-versioning is that "Exercises based on a canonical-data.json [...] should use its version plus a serial number.", so this all hinges on what the phrase based on means. I would accept "uses the problem-specifications tests, minus one of them" as acceptable for based on. We have also accepted "uses the problem-specifications tests, plus a few extras" as acceptable for based on, with an example being https://github.com/exercism/haskell/blob/master/exercises/difference-of-squares/test/Tests.hs#L29 and https://github.com/exercism/haskell/blob/master/exercises/difference-of-squares/package.yaml#L2. If we need to write in the README some rules for what is an acceptable standard for "based on", I'm all good with that.

My reason for why 2.1.0.3 is better: if there is ever a change 2.2.0 that adds a two-colour test case, we would likely want to add it to the track, and still be able to. So let's leave the version here so that tools can know whether this has happened. If we use 0.1.0.x I could certainly add it to the list of ignored exercises in https://github.com/petertseng/exercism-problem-specifications/blob/up-to-date/up-to-date/hs.rb#L3-L9, but then that script will just be blind to problem-specifications changes to resistor-color-duo.

For an example of where we specifically chose to ignore a problem-specifications file, see https://github.com/exercism/haskell/blob/master/exercises/triangle/test/Tests.hs whose version is 0.1.0.4 in https://github.com/exercism/haskell/blob/master/exercises/triangle/package.yaml#L2 , which is indeed not anything like https://github.com/exercism/problem-specifications/blob/up-to-date/exercises/triangle/canonical-data.json.

[sshine]

that script will just be blind to problem-specifications changes

I agree with this sentiment, and had it moments before coming this far in your review.

[sshine]

Another observation about the inaccuracy of README.md,

The program will take color names as input and output a two digit number

This is not true for any input (Black, x), in which the output will have one digit for implementations that does not preserve leading zeroes in the output. (I don't know of a numeric type in Haskell that does that, and I don't think other implementing tracks do this, either different tracks do this differently.)

I'll push a fix to highlight this in an issue on problem-specifications, since this is clearly a bug implemented in different ways, and there is no unit testing of the color black, and I don't know if that is deliberate.

[petertseng]

👍

I have a mind to document our omission from problem-specifications somewhere, but I am not sure if there is a good place. It doesn't seem necessary to convey the information to the student in the test file, yet it is the place where future editors of the test file would most likely see it. There does not seem to be a good place.

It seems to make more sense to squash these commits, but I would defer if you decide to not squash.

[sshine]

I've left a PR in exercism/problem-specifications#1605 as a reminder to address this eventually.

[sshine]

I have a mind to document our omission from problem-specifications somewhere

Ah, I understand what you mean now: Keep a note for when the version gets bumped again that states the reason for omission. One may realize that the data type does not allow it, but spelling it out lowers cognitive load and a required understanding of the context of present and past PRs.

bcf6125 adds this note to test/Tests.hs, but in such a way that necessitates me to push resistor-color-trio as an optional side-exercise available in close proximity to resistor-color-duo. I thought this constraint was a reasonable trade-off, since I've made that exercise without submitting it to the track.

I'll add [WIP] to this PR until I merge the other one, such that we don't point students to an exercise that isn't implemented.

[petertseng]

Ah, I understand now.

Given that I can't predict when resistor-color-trio will be added, I would also approve this following plan:

• Merge this PR, without the last commit
• Immediately re-submit the last commit as a separate PR, which is not to be merged until resistor-color-trio is added

The benefit of that is just that the improvements to this exercise aren't blocked on the adding of another.

[petertseng]

Actually I guess that resistor-color-trio will be made soon? Since you mentioned you have made it. I must not have read carefully. Okay, so that plan is only to be used in cases where you somehow experience a delay in getting resistor-color-trio ready for some reason.

[sshine]

Now that resistor-color-trio is ready in #869, this PR can subsequently be merged.