Simplify laws' implementations

[rpominov]

Follow up on the #193

I've only edited laws/applicative.js so far, but want to get an early feedback. Does this look good?

[rpominov]

Regarding whether this should be a breaking change. Strictly speaking we've newer documented the existence of this code, so it wasn't part of public API. Therefore maybe this is not a breaking change.

We should document it after refactoring though, I think. And after the refactoring it should became so simple so only new changes in this code should happen only when we change actual laws, add new algebras etc.

[safareli]

what about (A, eq) => (x,f) =>
First part contains stuff wich less likely will change and last part could change multiple times during property testing for example so this kind of partitioning is better imo

[rpominov]

I've separated them initially based on that eq will be required for any law and the rest of the arguments are different per each law. Was trying to unify API.

Not sure which way is better.

[davidchambers]

I'd be fine with eq => A => x => f =>. I don't like trying to predict how a function will be used. I think we should either take the arguments one at a time or all at once.

On a related note, we could drop the eq parameter if we were to depend on Z.equals. :)

[rpominov]

eq here is not necessarily fantasy-land/equals, it may not even return booleans. For example if we use this with Tasks eq may take two Tasks and return Task bool.

[rpominov]

Thinking a bit more (A, eq) => (x, f) => makes sense. We can make it that all functions take type representative and eq function for our type, and return a function that takes rest of dependencies for law equations.

Some examples:

// passing M for unification even though we don't need it for law equation
Monoid.rightIdentity = (M, eq) => (m) => ...

// passing T as usual even though it's not needed here, 
// other type representatives are passed 
// in the second chunk of arguments as other dependencies usually passed 
Traversable.composition = (T, eq) => (F, G, u) => ...

This way users may automatically partially apply all functions to specialize them for the type they want to test.

[joneshf]

What about moving that up further, to the exports?

// laws/monoid.js
const rightIdentity = (X, eq) => m => ...

module.exports = (X, eq) => {
  return {
    rightIdentity: rightIdentity(X, eq),
  };
};

// laws/traversable.js
const composition = (X, eq) => (F, G, u) => ...

module.exports = (X, eq) => {
  return {
    composition: composition(X, eq),
  };
};

// laws.js
const Monoid = require ('./laws/monoid');
const Traversable = require ('./laws/traversable');

module.exports = (X, eq) => {
  return {
    Monoid: Monoid(X, eq),
    Traversable: Traversable(X, eq),
  };
};

Or however it's supposed to be structured. Users can just import the top level laws thing, and unpack from there. That gives the added benefit that all the things are the same across laws. Or they can pull in piece meal if they need that for some reason–which is how it currently is.

[safareli]

@joneshf that's nice! +1

[rpominov]

What if we go even further and basically export just one function that accepts eq and X and returns all the law-checkers? In other words "top level laws thing" will be our only API.

[safareli]

As we just have one expression which is returned what about this:

- __ => {
-   return ..
- }
+ __ => ..

[rpominov]

Updated everything based on feedback. Still only couple of algebras added. Is this a good direction?

[safareli]

I think it's still good to have separate file for each spec so that libs could use specific one if desired, but the universal laws function is good as well.

Actually we can do both of them, we could have laws/index.js file which looks like this:

module.exports = (T, eq) => ({
  Setoid: require('./setoid')(T),
  Semigroup: require('./semigroup')(T, eq),
  ...
})

[rpominov]

It's about minimizing API surface area. There are benefits of having smaller API of a module, for example we can restructure stuff internally without affecting public API more easily. So maybe we should expose individual algebras, but there have to be a reason for that. We shouldn't do it just because we can.

[safareli]

I was thinking that maybe one would require only laws it needs, but as they are only needed in tests it can't have big benefits so I agree with you and now it lgtm!

but if I' only using Setoid why would I need to pass some eq?

[rpominov]

but if I' only using Setoid why would I need to pass some eq?

Good point. I guess one can just pass undefined both as T and eq when they only need Setoid, though.

[davidchambers]

Merging #193 introduced merge conflicts. Could you rebase and resolve them, @rpominov?

[rpominov]

Sure, there is a lot to be done here anyway. I'll get back to this soon.

[safareli]

Personally I don't quite like to pass undefineds to ./law.js. I think it's better to have separate file for each spec. so that we dont need to pass 'undefined's around

[rpominov]

What if we expose individual specs but keep everything in one file? It would be used like this:

const laws = require('fantasy-land/laws')

// Using an individual spec
laws.Setoid().reflexivity(...)

// Using all specs (passing undefined as T and eq)
laws.all().Setoid.reflexivity(...)

I still don't think we should expose individual specs though. This would be just more complicated API. Some specs require T and eq, some only eq, and Setoid doesn't need neither of them. One unified function looks like something that easier to understand and use.

Also we can change order of T and eq arguments, so with one function it could look like this:

const laws = require('fantasy-land/laws')

laws().Setoid.reflexivity(...)
laws(eq).Semigroup.associativity(...)
laws(eq, MyType).Applicative.identity(...)

[joneshf]

I don't like the idea of supporting undefined arguments. If a user doesn't want to use the eq argument, they should pass in (_, _) => true or whatever we were going to guess they meant.

Guessing the user's intention seems to go against the spirit of the spec. And, it shouldn't be our burden to support use cases like that.

[rpominov]

But we won't have to do anything special to support undefined. We will support it automatically. We don't use T and eq arguments in Setoid for example, so they can be whatever including undefined.

[rpominov]

So this boils down to how we document it. We can say in documentation that {} and (a, b) => true can be used as dummy T and eq, or we can say that one can just pass undefineds.

[safareli]

why wrap all laws in some function which sometimes needs T, sometimes eq and sometimes both of them, we can just expose a object like this:

module.exports = {
  Setoid: () => ({ ... }),
  Semigroup: (eq) => ({ ... }),
  Applicative: (T, eq) => ({ ... }),
}

And if in future, some spec needs T, eq and some other thing as well, we could freely add without bothering about the one universal all function arguments.

[rpominov]

@safareli this sounds like a reasonable solution.
@joneshf what do you think?

[rpominov]

Sorry, not sure I'll have free time to work on this, and I don't like to keep things hanging, so I'll close this for now.

If someone wants to pick this up, please do!