Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Fantasy land applicatives #315

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

pfgray
Copy link

@pfgray pfgray commented Sep 25, 2018

For #209

Just trying to collect some code around these implementations:

applyTo Tests Law tests
Async
Const
Either
IO
Identity
List
Maybe
Pair
Reader
Result
State
Unit
Writer

t.end()
})

// todo: What can we generalize for all Applicative law tests??
Copy link
Owner

@evilsoft evilsoft Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was thinking about setting something up for all law specs.
So it would be something like as a support file in the specs under src/test/laws.js:

const equals = require('../core/equals')

exports.setoid = {
  reflexivity: a => a.equals(a),
  symmetry: (a, b) => a.equals(b) === b.equals(a),
  transitivity: (a, b, c) => !(a.equals(b) && b.equals(c)) || a.equals(c),
}
 exports.semigroup = {
  associativity: (a, b, c) =>
    equals(a.concat(b).concat(c), a.concat(b.concat(c))),
}
 exports.monoid = {
  rightIdentity: (type, m) => equals(m.concat(type.empty()), m),
  leftIdentity: (type, m) => equals(type.empty().concat(m), m),
}

Then have another support file that checks for existing methods (like you need map for ap, etc).
If you would like to add the start (just for Apply and Applicative) that would be cool, if not just mimic how Apply and Applicative laws are tested for non-FL for now, then we can do a sweep and clean up all the other specs in a separate PR.

EDIT: Add equals ref to code.

EDIT: There is fantasy-laws but that puts a hard dependency around Sanctuary and only tests the fantasy-land/** methods. These laws are easy enough to verify so doing this ourselves.

Copy link
Owner

@evilsoft evilsoft Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I am thinking the laws file would look something like this:

const fl = require('../core/flNames')

const compose =
  f => g => x => f(g(x))

const identity =
  x => x

const applyTo =
  x => f => f(x)

module.exports = {
  apply: {
    composition: (equals, f, g, m) => equals(
      f.map(compose).ap(g).ap(m),
      f.ap(g.ap(m))
    )
  },

  'fl/apply': {
    composition: (equals, f, g, m) => equals(
      m[fl.ap](g[fl.ap](f[fl.map](compose))),
      m[fl.ap](g)[fl.ap](f)
    )
  },

  applicative: type => ({
    identity: (equals, x) => equals(
      type.of(identity).ap(x),
      x
    ),

    homomorphism: (equals, f, x) => equals(
      type.of(f).ap(type.of(x)),
      type.of(f(x))
    ),

    interchange: (equals, mFn, x) => equals(
      mFn.ap(type.of(x)),
      type.of(applyTo(x)).ap(mFn)
    )
  })
}

providing the equals function on each test for those cases that we do not have a Setoid (Async is not a Setoid in crocks. That way we can set up a t.plan(n) and use a function like:

const spec = (name, t) => (a, b) => {
  const aRes = sinon.spy()
  const bRes = sinon.spy()

  a.fork(unit, aRes)
  b.fork(unit, bRes)

  t.same(aRes.args[0], bRes.args[0], name)
}

for testing equality on all laws in the Async file. So the Apply laws (for non-fantasyland) would look like:

test('Async ap properties (Apply)', t => {
  t.plan(3)

  t.ok(isFunction(Async(unit).map), 'implements the Functor spec')
  t.ok(isFunction(Async(unit).ap), 'provides an ap function')

  const times10 = x => x * 10
  const plus10 = x => x + 10

  laws.apply.composition(
    spec('composition', t),
    Async.of(times10),
    Async.of(plus10),
    Async.of(0)
  )
})

But for cases like Maybe that are indeed Setoid and not lazy, etc, etc then it just looks like:

const equals = require('./compose')

test('Maybe ap properties (Apply)', t => {
  const j = Maybe.Just(3)
  const n = Maybe.Nothing()

  t.ok(isFunction(j.ap), 'Just provides an ap function')
  t.ok(isFunction(j.map), 'Just implements the Functor spec')

  t.ok(isFunction(n.ap), 'Nothing provides an ap function')
  t.ok(isFunction(n.map), 'Nothing implements the Functor spec')

  const f = x => x * 10
  const g = x => x + 10

  t.ok(laws.apply.composition(equals, Maybe.of(f), Maybe.of(g), j), 'composition Just')
  t.ok(laws.apply.composition(equals, Maybe.of(f), Maybe.of(g), n), 'composition Nothing')

  t.end()
})

But I am still not 💯 on that API tho.

Copy link
Author

@pfgray pfgray Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, I like the laws.{type}.{law_name} scheme 👍

I'll add the Apply/Applicative laws like you recommended in this PR, then we can add others in a follow-up

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for doing this, Super exciting!


const rejectOnce = once(reject)

// todo: not sure if this is necessary
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you are correct!
This is an old artifact. You can remove it from your applyTo. Leave it in ap for now and I will make an issue as that is already released to the world.


function resolveBoth() {
if (!cancelled && resolvedA && resolvedF) {
resolve(resolvedF(resolvedA))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this better then the compose(resolve, resolvedF)(resolvedA) that was implemented in the original ap. This is better for the stack on these lazy types.

Will also change these silly compositions in ap when we address the "pointless" cancelled flag.

@evilsoft
Copy link
Owner

evilsoft commented Oct 7, 2018

@pfgray when you get a chance you will wanna rebase against current master.
We have a fix in there for the spec runner on node:6

@pfgray
Copy link
Author

pfgray commented Oct 10, 2018

@evilsoft , it looks like there are 5 tests failing:

  Failed Tests: There were 5 failures
    isApplicative fantasy-land
      ✖ returns false when Applicative is passed

    isApply fantasy-land
      ✖ returns false when Apply is passed

    isChain fantasy-land
      ✖ returns false when Chain is passed

    isMonad fantasy-land
      ✖ returns false when Monad is passed

    isAlternative fantasy-land
      ✖ returns false when Alternative is passed

if I remove the addition of ap from flNames.js, then they pass.

I'm not quite sure what they're testing, or how these should behave now that we have fantasy-land/ap defined on these.

@evilsoft
Copy link
Owner

@pfgray will look into that, In the end, all of the pointfree functions should call the fantasy land function first, then dispatch to the non-prefixed. will pull down the branch and see what is going on with dem specs.

@evilsoft
Copy link
Owner

@pfgray I am not seeing those same failures locally.
What environment are you seeing them in?

@pfgray
Copy link
Author

pfgray commented Oct 13, 2018

@evilsoft, ah yes, sorry... I pushed some changes to the tests in this commit since without these changes those tests fail.

Are the isApplicative methods supposed to return false if a constructor or instance of a "fantasy-land applicative" is passed? Or is it just used to determine what is an "applicative" wrt the crocks library?

Also, would this then be considered a "breaking" change?

@evilsoft
Copy link
Owner

evilsoft commented Oct 14, 2018

Ahh. sorry I misunderstood what was going on here.
Yes that will be a breaking change, and TBH updating the predicates is out of scope for this PR. it should really just handle the non-breaking changes of adding applyTo to the types and even the fantasy-land/ap.

There are a few breaking changes that will be happening around this and traversable.
While those specs are valid for apply and applicative, I am worried about chain and monad and alternative. Those should have been true. Digging into those right now.

EDIT:
Ohhh. nope at the present time those specs are indeed valid and it looks like we are going to have to do this in a breaking change. All of those methods depend on the type being an Apply first, or it appears along the chain (monad needs to be an chain and an applicative, and both chain and applicative must be an apply) So by adding the method fantasy-land/ap, it then reports true all the way down.

So to answer your question, yes this will be a breaking change and yes it is expected, I just did not anticipate it. So sorry about the 🌽fusion.

@evilsoft
Copy link
Owner

And do not sweat the breaking change, we have a couple of them coming down the pipe (Fix to Const and Writer. @bennypowers esm changes as a big change on the List ADT).

@evilsoft
Copy link
Owner

Also another thing I forgot to mention, each ADT has a VERSION variable at the top and an associated spec. This marks an API version for integration with sanctuary-js, so as the API is being updated, those versions will also need to be incremented.

@dalefrancis88
Copy link
Collaborator

@evilsoft @pfgray What do we need to get this merged in? I'm happy to pickup the final touches if you guys just let me know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants