adds ord #10
adds ord #10
Conversation
|
Reminder for myself: |
|
I added some examples and changed |
| const {Max, Min, Ord} = require('../fantasy-monoids'); | ||
| const log = (x) => console.log(x); | ||
| const concat = (x, y) => x.concat(y); | ||
| const mconcat = (xs, empty) => xs.length ? xs.reduce(concat) : empty(); |
There was a problem hiding this comment.
@SimonRichardson mconcat would be a great helper. What say?
Should something like this go to fantasy-helpers or stay with the monoids?
|
Not related to this PR but it would be nice to not spread the use of nodeunit further which is just entirely dead. |
Agreed. |
| @@ -0,0 +1,42 @@ | |||
| const {Max, Min, Ord} = require('../fantasy-monoids'); | |||
There was a problem hiding this comment.
Instead of this example I would prefer some unit tests that convey information on usage and behaviour. Unlike unit tests the example also does not contain the output so you have to actually run it and play with it to even know what the result is for sure.
Note that the fantasy checks merely ensure that certain laws are adhered to. That is something quite different.
There was a problem hiding this comment.
Jop, Will add some unit tests and move to mocha. @SimonRichardson okay with you?
There was a problem hiding this comment.
... or maybe I will just add some unit tests and do the move to mocha later. Currently all of the fl repos use nodeunit. I think when we decide to move to mocha ore something else, we should cleanup all repos.
There was a problem hiding this comment.
I would suggest to do it now because otherwise you are just creating additional tech debt and make the eventual switch more painful because there is yet another project that needs to be taken care of ;-)
Just my 2cents as an "outsider" ;)
|
I moved the examples to the tests and added mconcat/ concat |
|
I think this is ready to 🚢 . I consider moving to mocha in a separate PR (I'm totally in favour of that, never was a fan of nodeunit.). @SimonRichardson anything to change, add? do you want me to squash the commits, or okay like this? |
|
I'd rather stick with nodeunit because the rest of the libraries are, so if we change we should change all of them. So 👎 for mocha atm |
|
Works for me. |
stoeffel
force-pushed
the
ord-experimental
branch
2 times, most recently
from
3e9e768
to
450585b
Compare
stoeffel
force-pushed
the
ord-experimental
branch
from
450585b
to
5a0c706
Compare
|
rebased and ready to 🚢 IMO. |
|
OK I'll have a look tomorrow. On Mon, 22 Feb 2016, 20:26 Christoph Hermann notifications@github.com
|
|
Cool. let me know if I should change or add something. |
|
Sure, will do On Mon, 22 Feb 2016, 20:28 Christoph Hermann notifications@github.com
|
|
did you have time to review this? anything to add? no need to hurry just want to check in. |
|
👍 Sorry for the delay, been really busy atm. |
no problem :-) Thanks for merging. |
This adds a
Ordtype and makesMinandMaxwork with it. I will clean this up next week.