Implementing: complex-numbers by Average-user · Pull Request #601 · exercism/haskell

Average-user · 2017-10-04T19:13:22Z

No description provided.

Average-user · 2017-10-04T19:52:43Z

How can I check the Travis build before upload?

petertseng · 2017-10-04T21:15:38Z

How can I check the Travis build before upload?

You may see the exact commands that the travis runs by looking at https://github.com/exercism/haskell/blob/master/.travis.yml#L23-L49 . You may run any or all of those commands on your own computer.

Average-user · 2017-10-05T00:08:49Z

@petertseng Also, there is a way to don't have to follow the HLints suggestions? Sometimes the extra parentheses can help to read something, I believe.

petertseng · 2017-10-05T00:16:09Z

don't have to follow the HLints suggestions

https://github.com/exercism/haskell/blob/master/README.md#running-hlint

Average-user · 2017-10-07T01:03:48Z

@petertseng I want your feedback about if it is Float the right type for this, maybe, I don't really know. Or maybe is it better tu use mixed types

petertseng · 2017-10-07T01:32:06Z

if it is Float the right type for this

I'm not sure I understood the question correctly. I think my answer is "as long as it passes the tests, it doesn't matter what the internal implementation details are", right?

petertseng

good stuff.

petertseng · 2017-10-10T01:37:22Z

+      "core": false,
+      "unlocked_by": null,
+      "difficulty": 2,
+      "topics": [


I think I will feel the urge to remove this. I will feel bad having this exercise with "mathematics" topic, and none of the other exercises that should have "mathematics" topic have it. It isn't one that would affect a placement decision (the topics added in #476 ).

I guess I would accept a PR that adds the mathematics topic to all the exercises that should have it.

Maybe I'll do that.

petertseng · 2017-10-10T01:37:48Z

+      "unlocked_by": null,
+      "difficulty": 2,
+      "topics": [
+        "Mathematics",


will any other exercise have this topic? if so, I'd like to see it added to them as well. If not, I'm not finding it particularly useful

petertseng · 2017-10-10T01:38:44Z

+ sub,
+ div) where
+
+import Prelude hiding (div, abs) -- you have to redifine this two functions


I don't think the comment is necessary since we did not use it in any other exercise that has hiding, but if it is, it would have to be spelled "redefine these two functions"

petertseng · 2017-10-10T01:40:01Z

+import Prelude hiding (div, abs) -- you have to redifine this two functions
+
+-- Data definition -------------------------------------------------------------
+data Complex = Complex Float Float deriving (Eq, Show)


I would not put this here, because students might think they are not allowed to change it. I would like to let the students have their creativity, to choose what they like. So, this line would probably have to be data Complex = Dummy deriving (Eq, Show). Maybe even data Complex = Dummy if we can get away with it.

But what will happen if someone uses a definition like data Complex = Complex double double I think that would be ok, but the tests wouldn't work. I think.

That's true. You could look at https://github.com/exercism/haskell/blob/master/exercises/space-age/test/Tests.hs if you want help with that, I suppose. There are example https://github.com/exercism/haskell/blob/master/exercises/space-age/examples/success-double/src/SpaceAge.hs which uses doubles and https://github.com/exercism/haskell/blob/master/exercises/space-age/examples/success-rational/src/SpaceAge.hs which uses rational - both compile with those tests. You could possibly do something similar.

petertseng · 2017-10-10T01:43:03Z

+  testC f CaseC{..} = it descriptionC $ f number1C `shouldBe` expectedC
+
+
+data CaseA = CaseA { descriptionA :: String


Possible to have more descriptive names than A, B, C?

BinOp, UnOp, Scalar?

Could, be. Don't know if it is worth with something this simple.

petertseng · 2017-10-10T01:43:18Z

+            , number1B     = Complex 0 5
+            , expectedB    = Complex 0 (-5)
+            }
+    , CaseB { descriptionB = "Conjugate a number with real and  imaginary part"


extra space between "and" and "imaginary"

petertseng · 2017-10-10T01:43:48Z

@@ -0,0 +1 @@
+# Pls regenerate this


a reminder to myself to regenerate it before merging.

Average-user · 2017-10-10T20:36:03Z

Don't really know why it fails. When I run bin/test-all-examples exercises/complex-numbers/ works perfectly, but I'm guessing that has to do with the Complex = Dummy definition.

petertseng · 2017-10-10T21:22:34Z

Don't really know why it fails

Why not? Will you explain what part of https://travis-ci.org/exercism/haskell/jobs/286216812 you did not understand?

When I run bin/test-all-examples exercises/complex-numbers/ works perfectly

So that means the example is good.

but I'm guessing that has to do with the Complex = Dummy definition.

Then you should read what https://travis-ci.org/exercism/haskell/jobs/286216812 says, and/or run stack test to test the stub, and/or run https://github.com/exercism/haskell/blob/master/bin/test-stub.

As you can see, Travis says

/home/travis/build/exercism/haskell/build/complex-numbers/stub/test/Tests.hs:48:30: error:
    Data constructor not in scope:
      Complex :: Integer -> Integer -> Complex

Indeed, there is no such data constructor, since it's Complex = Dummy.

So, how about a Integer -> Integer -> Complex function (maybe called complex) that the student has to implement? Then the test can use that function instead of using Complex (and thus forcing Complex to take Integer -> Integer as well). This is continuing the trend of allowing the internal representation to be changed at will.

Also, you're probably wondering why I didn't just tell you to do this at the same time as changing to Complex = Dummy.

It's because I forgot about it. Sorry for my bad memory.

petertseng · 2017-10-10T21:23:53Z

+  testC f CaseC{..} = it descriptionC $ f number1C `shouldBe` fromInteger expectedC
+
+
+data CaseA = CaseA { descriptionA :: String


I'll think about the names

Average-user · 2017-10-10T21:47:14Z

@petertseng Maybe it was fine as it was before. Cause you say that a function Integer -> Integer -> Complex Will force to take Complex with Integer -> Integer.

But on this section of code:

real :: Complex -> Float
real = error "You need to implement this function"

imaginary :: Complex -> Float
imaginary = error "You need to implement this function"

It was already kind of implicit the Complex with Float -> Float. So, taking that in count it might still be a good idea to just define data Complex = Complex Float Float

I believe.

petertseng · 2017-10-10T22:09:58Z

an interesting question will then be whether it makes sense to let it be a parameterized type, such as https://hackage.haskell.org/package/base/docs/Data-Complex.html has Complex a, and then it has realPart :: Complex a -> a and imagPart :: Complex a -> a. That could be considered.

Average-user · 2017-10-10T22:42:58Z

You mean data Complex a = Complex a a ? If thats the case, what would be the better thing to put on the src/ComplexNumbers.hs definitions? Should be data Complex a = Complex a a given?

petertseng · 2017-10-10T23:14:57Z

You mean data Complex a = Complex a a?

You can choose what goes in the example. What you said is a possibility.

what would be the better thing to put on the src/ComplexNumbers.hs definitions? Should be data Complex a = Complex a a given?

I still think it should be data Complex a = Dummy, see all instances of this in this repo

$ git grep Dummy            (10-10 23:13)
exercises/bank-account/src/BankAccount.hs:data BankAccount = Dummy
exercises/binary-search-tree/src/BST.hs:data BST a = Dummy deriving (Eq, Show)
exercises/clock/src/Clock.hs:data Clock = Dummy
exercises/custom-set/src/CustomSet.hs:data CustomSet a = Dummy deriving (Eq, Show)
exercises/forth/src/Forth.hs:data ForthState = Dummy
exercises/grade-school/src/School.hs:data School = Dummy
exercises/linked-list/src/Deque.hs:data Deque a = Dummy
exercises/matrix/src/Matrix.hs:data Matrix a = Dummy deriving (Eq, Show)
exercises/robot-name/src/Robot.hs:data Robot = Dummy
exercises/robot-simulator/src/Robot.hs:data Robot = Dummy
exercises/simple-linked-list/src/LinkedList.hs:data LinkedList a = Dummy deriving (Eq, Show)
exercises/zipper/src/Zipper.hs:data Zipper a = Dummy deriving (Eq, Show)

Average-user · 2017-10-10T23:55:33Z

But if I leave the Complex a = Dummy definition, the previous error will remain.

petertseng · 2017-10-10T23:59:43Z

But if I leave the Complex = Dummy the previous error will remain.

So have a function that creates a Complex a from two a, and make the tests use it.

petertseng · 2017-10-11T01:07:55Z

 -- binary operators ------------------------------------------------------------
-mul :: Complex -> Complex -> Complex
+complex :: (a, a) -> Complex a
+complex (a, b) = Complex a b


~~What about complex = curry Complex ?~~

Oh sorry, that needs to be complex = uncurry Complex, get them mixed up too often.

Forgot about that. I don't use neither of them too often.

petertseng · 2017-10-11T01:11:16Z


 -- binary operators ------------------------------------------------------------
-mul :: Complex -> Complex -> Complex
+complex :: (a, a) -> Complex a


I wouldn't put this in binary operators section. Binary operator implies Complex a -> Complex a -> something. This complex creates a complex number.

Binary operator implies two arguments. But I see what you mean.

petertseng · 2017-10-11T08:18:34Z

I think it is good now after I regenerate the README. I may need some days to do it.

Average-user · 2017-10-11T10:04:02Z

Untill them, then.

petertseng

As you can see, I generated the README. I also made two other changes that I might have considered asking you to make, but I had already made you wait too longer:

After using the complex function, no longer any need to export Complex(..), just Complex.
The files shouldn't be executable, but they all were. Let's not do that for the next PR.

petertseng · 2017-10-13T04:03:25Z

All right, thanks for your fourth exercise contribution!

Average-user · 2017-10-13T04:56:42Z

@petertseng

The files shouldn't be executable, but they all were. Let's not do that for the next PR.

Don't really understand what you mean with that. Also your last commit appears to have zero changes.

petertseng · 2017-10-13T05:28:27Z

Also your last commit appears to have zero changes.

I don't see any commit with no changes. I see my last commit is f44dd15 but there are changes, all modes changed from 100755 to 100644 (meaning I made them no longer executable).

Average-user · 2017-10-13T05:32:05Z

@petertseng

I don't see any commit with no changes. I see my last commit is f44dd15 but there are changes, all modes changed from 100755 to 100644.

Right didn't see that. My bad.

petertseng · 2017-10-15T22:52:15Z

      "topics": [
      ]
    },
+    {


note: check whether g is OK for uuid

AFAIK UUID is usually 16 byte written in hexadecimal notation and separated into 5 groups. So it doesn't look like a valid UUID to me.

Wikipedia: (https://en.wikipedia.org/wiki/Universally_unique_identifier#Format)

In its canonical textual representation, the sixteen octets of a UUID are represented as 32 hexadecimal (base 16) digits, displayed in five groups separated by hyphens, in the form 8-4-4-4-12 for a total of 36 characters (32 alphanumeric characters and four hyphens).

Has this UUID been generated by configlet?

I've opened a bug in the configlet repo.

Wether generated or not, at least the linter should have complained:

exercism/configlet#100

@NobbZ It wasn't generated by configlet, my bad there. Also you might want to specify in the issue that it has already been fixed the UUID of this repo.

petertseng reviewed Oct 10, 2017

View reviewed changes

petertseng reviewed Oct 11, 2017

View reviewed changes

Average-user and others added 10 commits October 12, 2017 20:51

Implementing: complex-numbers

ac22322

Travis suggestions

bb105fc

correction on src/ComplexNumbers.hs

63f1f82

Update ComplexNumbers.hs

68f2521

ComplexNumbers.hs update

c27613f

some correction

3355c20

Tests.hs update

d6584a9

parameterized type

a872e2a

little changes in the example

dc3bce7

Update ComplexNumbers.hs

1d5f0a4

petertseng added 3 commits October 12, 2017 20:52

complex-numbers: generate README

11149e4

no longer need to expose Complex ctors

7f5f1b0

no +x on files

f44dd15

petertseng approved these changes Oct 13, 2017

View reviewed changes

petertseng merged commit e462065 into exercism:master Oct 13, 2017

petertseng reviewed Oct 15, 2017

View reviewed changes

petertseng mentioned this pull request Oct 15, 2017

implementing rotational-cipher #604

Merged

NobbZ mentioned this pull request Oct 16, 2017

lint: invalid UUIDs seem to pass exercism/v2-configlet#100

Closed

Average-user deleted the complex_implement branch October 23, 2017 20:00

MatthijsBlom mentioned this pull request Jan 18, 2024

Added Rational Numbers exercise #1193

Merged

		testC f CaseC{..} = it descriptionC $ f number1C `shouldBe` expectedC


		data CaseA = CaseA { descriptionA :: String

Uh oh!

Conversation

Average-user commented Oct 4, 2017

Uh oh!

Average-user commented Oct 4, 2017

Uh oh!

petertseng commented Oct 4, 2017

Uh oh!

Average-user commented Oct 5, 2017

Uh oh!

petertseng commented Oct 5, 2017

Uh oh!

Average-user commented Oct 7, 2017

Uh oh!

petertseng commented Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petertseng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Average-user Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Average-user commented Oct 10, 2017

Uh oh!

petertseng commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Average-user commented Oct 10, 2017

Uh oh!

petertseng commented Oct 10, 2017

Uh oh!

Average-user commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petertseng commented Oct 10, 2017

Uh oh!

Average-user commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petertseng commented Oct 10, 2017

Uh oh!

petertseng Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petertseng commented Oct 11, 2017

Uh oh!

Average-user commented Oct 11, 2017

Uh oh!

petertseng left a comment

petertseng commented Oct 7, 2017 •

edited

Loading

Average-user Oct 10, 2017 •

edited

Loading

petertseng commented Oct 10, 2017 •

edited

Loading

Average-user commented Oct 10, 2017 •

edited

Loading

Average-user commented Oct 10, 2017 •

edited

Loading

petertseng Oct 11, 2017 •

edited

Loading

petertseng commented Oct 13, 2017 •

edited

Loading

NobbZ Oct 16, 2017 •

edited

Loading