mi-afp / hw05 Goto Github PK

Hi! 🙂 I think I have found several mistakes in the test suite for RomanNumeral in HW-05. I will post my findings bellow as well as create PR with the proposed fix.

• unpack (RomanNumeral "D") shouldBe 100 -> should be 500

• unpack (RomanNumeral "-CCXV") shouldBe 215 -> should be (-215)

• unpack (RomanNumeral "") shouldBe 0
  ~ I believe this is correct

• evaluate (unpack (RomanNumeral "")) shouldThrow errorCall "Illegal Roman Numeral: ''"
  ~ I believe this is not

• (RomanNumeral "MCCXL" + RomanNumeral "-MM") shouldBe RomanNumeral "DCCLX"
  ~ I believe that 1240 - 2000 = -760 and not 760 as stated in test -> therefore should be "-CCLX"

• (RomanNumeral "MCCXL" - RomanNumeral "MM") shouldBe RomanNumeral "DCCLX"
  ~ From same reasons as above I believe this should be "-DCCLX"

• (RomanNumeral "" - RomanNumeral "VII") shouldBe RomanNumeral "VII"
  ~ Similar reasons, this should be 0 - 7 = -7 and therefore "-VII"

• All tests in "is Integral: can compute quotient and remainder" attempt to divide by zero, which results in an exception being thrown (it seems author forgot to add in values)

• toInteger (RomanNumeral "-DCCLXXXV") shouldBe 785 is once again negative number and therefore the result should be (-785)

it "matches with AnyInternal" $ do
  (AnyInternal ~~ esInternal1) `shouldBe` True
  (AnyInternal ~~ esExternal)  `shouldBe` False
  (AnyInternal ~~ esCombined)  `shouldBe` True
  (AnyInternal ~~ Unknown)     `shouldBe` False
  (AnyInternal ~~ Combined [esExternal,Unknown]) `shouldBe` False
it "matches with AnyInternal" $ do
  (AnyExternal ~~ esInternal1) `shouldBe` False
  (AnyExternal ~~ esExternal)  `shouldBe` True
  (AnyExternal ~~ esCombined)  `shouldBe` True
  (AnyExternal ~~ Unknown)     `shouldBe` False
  (AnyExternal ~~ Combined [esInternal1,Unknown]) `shouldBe` False
it "matches with AnyInternal" $ do
  (Any ~~ esInternal1) `shouldBe` True
  (Any ~~ esExternal)  `shouldBe` True
  (Any ~~ esCombined)  `shouldBe` True
  (Any ~~ Unknown)     `shouldBe` True
  (Any ~~ Combined [esInternal1,Unknown]) `shouldBe` True

copy-paste typos in titles

Hi! 🙂 I have looked on your fixed version of tests and I would like you to specify the definition of your modulo arithmetic.

As far as I have been taught, the modulo of n must be in range from 0 to n - 1. However, according to your tests, this does not hold - see

(RomanNumeral "-IV" quotRem RomanNumeral "III") shouldBe (RomanNumeral "-I", RomanNumeral "-I") ~ (-4 mod 3 = -1) ~ I would say it should be 2.

Also I doubt anyone in any of our math courses have even used a negative number as base for modulo operation, but then I would say the result should be in range from n + 1 to 0. However, the tests state following

(RomanNumeral "X" quotRem RomanNumeral "-III") shouldBe (RomanNumeral "-III", RomanNumeral "I") ~ (10 mod -3 = 1) ~ I (and even Prelude) say that it should be -2.

Could you please clarify this? Thank you 🙂