search

fantasyland / fantasy-monoids

A collection of monoids
29 stars 8 forks source link

adds ord #10

Closed stoeffel closed 8 years ago

stoeffel commented 8 years ago

This adds a Ord type and makes Min and Max work with it. I will clean this up next week.

stoeffel commented 8 years ago

Reminder for myself: To define a Ord one should have to define max/min not empty. Max should use Ord.max and Min -> Ord.Min

stoeffel commented 8 years ago

I added some examples and changed Ord to have min/max instead of empty (see examples). The only thing that I don't like of the current solution is that you end up with: wrapped { x: wrapped { x: -8 } }

gilligan commented 8 years ago

Not related to this PR but it would be nice to not spread the use of nodeunit further which is just entirely dead.

stoeffel commented 8 years ago

Not related to this PR but it would be nice to not spread the use of nodeunit further which is just entirely dead.

Agreed.

stoeffel commented 8 years ago

I moved the examples to the tests and added mconcat/ concat

stoeffel commented 8 years ago

I think this is ready to :ship: . 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?

SimonRichardson commented 8 years ago

I'd rather stick with nodeunit because the rest of the libraries are, so if we change we should change all of them. So :-1: for mocha atm

stoeffel commented 8 years ago

Works for me.

stoeffel commented 8 years ago

rebased and ready to :ship: IMO.

SimonRichardson commented 8 years ago

OK I'll have a look tomorrow.

On Mon, 22 Feb 2016, 20:26 Christoph Hermann notifications@github.com wrote:

rebased and ready to [image: :ship:] IMO.

— Reply to this email directly or view it on GitHub https://github.com/fantasyland/fantasy-monoids/pull/10#issuecomment-187369371 .

stoeffel commented 8 years ago

Cool. let me know if I should change or add something.

SimonRichardson commented 8 years ago

Sure, will do

On Mon, 22 Feb 2016, 20:28 Christoph Hermann notifications@github.com wrote:

Cool. let me know if I should change or add something.

— Reply to this email directly or view it on GitHub https://github.com/fantasyland/fantasy-monoids/pull/10#issuecomment-187369972 .

stoeffel commented 8 years ago

did you have time to review this? anything to add? no need to hurry just want to check in.

SimonRichardson commented 8 years ago

:+1: Sorry for the delay, been really busy atm.

stoeffel commented 8 years ago

Sorry for the delay, been really busy atm

no problem :-) Thanks for merging.