From c05e826d2d890b80f1ddbf82e6d28399829a81a4 Mon Sep 17 00:00:00 2001 From: Simon Shine Date: Sat, 12 Oct 2019 05:28:36 +0200 Subject: [PATCH 1/4] resistor-color-duo: Change to `value :: (Color, Color) -> Int` 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 exercism/problem-specifications#1466 - 2019-03-12: exercise ported to Haskell track in #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 exercism/problem-specifications#1569. - 2019-10-01: third-band test case added to Haskell track in #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. --- .../success-standard/src/ResistorColors.hs | 28 +++++++++---------- exercises/resistor-color-duo/package.yaml | 2 +- .../resistor-color-duo/src/ResistorColors.hs | 6 ++-- exercises/resistor-color-duo/test/Tests.hs | 14 ++++------ 4 files changed, 23 insertions(+), 27 deletions(-) diff --git a/exercises/resistor-color-duo/examples/success-standard/src/ResistorColors.hs b/exercises/resistor-color-duo/examples/success-standard/src/ResistorColors.hs index f337d6fd7..c6ba973cb 100644 --- a/exercises/resistor-color-duo/examples/success-standard/src/ResistorColors.hs +++ b/exercises/resistor-color-duo/examples/success-standard/src/ResistorColors.hs @@ -11,19 +11,19 @@ data Color = | Violet | Grey | White - deriving (Eq, Show, Read) + deriving (Eq, Show) -convert :: Color -> Int -convert Black = 0 -convert Brown = 1 -convert Red = 2 -convert Orange = 3 -convert Yellow = 4 -convert Green = 5 -convert Blue = 6 -convert Violet = 7 -convert Grey = 8 -convert White = 9 +value1 :: Color -> Int +value1 Black = 0 +value1 Brown = 1 +value1 Red = 2 +value1 Orange = 3 +value1 Yellow = 4 +value1 Green = 5 +value1 Blue = 6 +value1 Violet = 7 +value1 Grey = 8 +value1 White = 9 -value :: [Color] -> Int -value = read . concatMap (show . convert) . take 2 +value :: (Color, Color) -> Int +value (a, b) = 10 * value1 a + value1 b diff --git a/exercises/resistor-color-duo/package.yaml b/exercises/resistor-color-duo/package.yaml index 84558d0d4..771ac33b7 100644 --- a/exercises/resistor-color-duo/package.yaml +++ b/exercises/resistor-color-duo/package.yaml @@ -1,5 +1,5 @@ name: resistor-color-duo -version: 2.1.0.2 +version: 0.1.0.3 dependencies: - base diff --git a/exercises/resistor-color-duo/src/ResistorColors.hs b/exercises/resistor-color-duo/src/ResistorColors.hs index e6c5dcdae..7474285be 100644 --- a/exercises/resistor-color-duo/src/ResistorColors.hs +++ b/exercises/resistor-color-duo/src/ResistorColors.hs @@ -11,7 +11,7 @@ data Color = | Violet | Grey | White - deriving (Eq, Show, Read) + deriving (Eq, Show) -value :: [Color] -> Int -value cs = error "You need to implement this function." +value :: (Color, Color) -> Int +value (a, b) = error "You need to implement this function." diff --git a/exercises/resistor-color-duo/test/Tests.hs b/exercises/resistor-color-duo/test/Tests.hs index 615dab1af..94c1e7050 100644 --- a/exercises/resistor-color-duo/test/Tests.hs +++ b/exercises/resistor-color-duo/test/Tests.hs @@ -20,29 +20,25 @@ specs = describe "value" $ for_ cases test assertion = value input `shouldBe` expected data Case = Case { description :: String - , input :: [Color] + , input :: (Color, Color) , expected :: Int } cases :: [Case] cases = [ Case { description = "Brown and black" - , input = [Brown, Black] + , input = (Brown, Black) , expected = 10 } , Case { description = "Blue and grey" - , input = [Blue, Grey] + , input = (Blue, Grey) , expected = 68 } , Case { description = "Yellow and violet" - , input = [Yellow, Violet] + , input = (Yellow, Violet) , expected = 47 } , Case { description = "Orange and orange" - , input = [Orange, Orange] + , input = (Orange, Orange) , expected = 33 } - , Case { description = "Ignore additional colors" - , input = [Green, Brown, Orange] - , expected = 51 - } ] From 249d26c7f25492bf4b53d16e34c496e41fcf4f16 Mon Sep 17 00:00:00 2001 From: Simon Shine Date: Sun, 13 Oct 2019 19:05:11 +0200 Subject: [PATCH 2/4] Add hint and change version back to 2.1.0.3 (was 2.1.0.2) --- exercises/resistor-color-duo/.meta/hints.md | 10 ++++++++++ exercises/resistor-color-duo/README.md | 12 ++++++++++++ exercises/resistor-color-duo/package.yaml | 2 +- 3 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 exercises/resistor-color-duo/.meta/hints.md diff --git a/exercises/resistor-color-duo/.meta/hints.md b/exercises/resistor-color-duo/.meta/hints.md new file mode 100644 index 000000000..055f0d15e --- /dev/null +++ b/exercises/resistor-color-duo/.meta/hints.md @@ -0,0 +1,10 @@ +## Hints + +You need to implement the function + +```haskell +value :: (Color, Color) -> Int +``` + +that only accepts two colors and produces the resistor's numeric value +component of its resistance. diff --git a/exercises/resistor-color-duo/README.md b/exercises/resistor-color-duo/README.md index ff41fd76f..e774b45b4 100644 --- a/exercises/resistor-color-duo/README.md +++ b/exercises/resistor-color-duo/README.md @@ -27,6 +27,18 @@ brown-green should return 15 brown-green-violet should return 15 too, ignoring the third color. +## Hints + +You need to implement the function + +```haskell +value :: (Color, Color) -> Int +``` + +that only accepts two colors and produces the resistor's numeric value +component of its resistance. + + ## Getting Started diff --git a/exercises/resistor-color-duo/package.yaml b/exercises/resistor-color-duo/package.yaml index 771ac33b7..525c90324 100644 --- a/exercises/resistor-color-duo/package.yaml +++ b/exercises/resistor-color-duo/package.yaml @@ -1,5 +1,5 @@ name: resistor-color-duo -version: 0.1.0.3 +version: 2.1.0.3 dependencies: - base From bcf61256fbbe68725de141ab673a023914c67204 Mon Sep 17 00:00:00 2001 From: Simon Shine Date: Mon, 14 Oct 2019 01:25:10 +0200 Subject: [PATCH 3/4] Add note in test file explaining omission of test This note is designed in such a way that both track maintainers and students can make use of it: The maintainer will know why not to add the missing test when bumping versions, and the student will know why the current README.md talks about an impossible third color band. --- exercises/resistor-color-duo/test/Tests.hs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/exercises/resistor-color-duo/test/Tests.hs b/exercises/resistor-color-duo/test/Tests.hs index 94c1e7050..5b917acd0 100644 --- a/exercises/resistor-color-duo/test/Tests.hs +++ b/exercises/resistor-color-duo/test/Tests.hs @@ -41,4 +41,7 @@ cases = [ Case { description = "Brown and black" , input = (Orange, Orange) , expected = 33 } + -- Note: This test suite omits testing three-color bands, + -- since they are not representable as (Color, Color). They + -- are addressed in the exercise resistor-color-trio. ] From 826de2a1f93f75ed074208305c5588128b0128f3 Mon Sep 17 00:00:00 2001 From: Simon Shine Date: Mon, 4 Nov 2019 14:17:05 +0100 Subject: [PATCH 4/4] Rename 'value1' back to 'convert' This makes the PR less invasive. --- .../success-standard/src/ResistorColors.hs | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/exercises/resistor-color-duo/examples/success-standard/src/ResistorColors.hs b/exercises/resistor-color-duo/examples/success-standard/src/ResistorColors.hs index c6ba973cb..8519158ef 100644 --- a/exercises/resistor-color-duo/examples/success-standard/src/ResistorColors.hs +++ b/exercises/resistor-color-duo/examples/success-standard/src/ResistorColors.hs @@ -13,17 +13,17 @@ data Color = | White deriving (Eq, Show) -value1 :: Color -> Int -value1 Black = 0 -value1 Brown = 1 -value1 Red = 2 -value1 Orange = 3 -value1 Yellow = 4 -value1 Green = 5 -value1 Blue = 6 -value1 Violet = 7 -value1 Grey = 8 -value1 White = 9 +convert :: Color -> Int +convert Black = 0 +convert Brown = 1 +convert Red = 2 +convert Orange = 3 +convert Yellow = 4 +convert Green = 5 +convert Blue = 6 +convert Violet = 7 +convert Grey = 8 +convert White = 9 value :: (Color, Color) -> Int -value (a, b) = 10 * value1 a + value1 b +value (a, b) = 10 * convert a + convert b