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

Fix id_test and argument order in laws #193

Merged
merged 3 commits into from Oct 27, 2016

Conversation

safareli
Copy link
Member

@safareli safareli commented Oct 25, 2016

related to #173 #171

some functions which need more arguments will silently fail as they return function which passes `t.ok` check
@@ -1,5 +1,6 @@
'use strict';

const fl = require('.');
Copy link
Member

Choose a reason for hiding this comment

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

Could you change the path to './'? '.' has always worked for me, but I've seen it fail for other people.

@davidchambers
Copy link
Member

It's pretty clear no one is using these at the moment. ;)

@safareli
Copy link
Member Author

safareli commented Oct 25, 2016

i'm using it but just updated from v1 to v2 :p

@rpominov
Copy link
Member

I wonder if maybe these should live in a separate project. Here are couple benefits:

@davidchambers
Copy link
Member

The version number of fantasy-land package will only track changes in the spec itself.

I find this compelling. It'd be a shame to keep incrementing the spec's version in order to release fixes such as these, but it'd be unreasonable to force such fixes to wait until a new version of the spec is released. Allowing the two projects to be released independently strikes me as a good idea.

@safareli
Copy link
Member Author

I think changes like this should bump patch version for now.

@davidchambers
Copy link
Member

🌳

@SimonRichardson
Copy link
Member

I still think the laws should be in this repo, but I think they should be dramatically improved (or reworked, refactored, re-written). They're very old, but are worth their gold when they did work. Personally we should go one step further and make them links in the spec, so people are more likely to know about them.

@rpominov
Copy link
Member

rpominov commented Oct 26, 2016

I think laws should look like this:

// `u.map(a => a)` is equivalent to `u` (identity)
const identity = (u) => {
  return {
    left: u[map](x => x),
    right: u,
  }
}

// `u.map(x => f(g(x)))` is equivalent to `u.map(g).map(f)` (composition)
const composition = (u, f, g) => {
  return {
    left: u[map](x => f(g(x))),
    right: u[map](g)[map](f),
  }
}

When we're trying to make them smarter, they just end up incomplete or hard to use with certain types.

Also this simple laws implementations will be easy to keep up to date because it's basically copy-paste of the snippets from spec.

I can make a PR that simplifies laws like this if this sounds like a good idea.

@safareli
Copy link
Member Author

safareli commented Oct 26, 2016

const identity = (u) => {
  return {
    left: u[map](x => x),
    right: u,
  }
}
{left,right} = identity(Maybe.Just(1))
maybeEquals(left,right)

i don't see a big difference with this

const identity = (eq, u) => eq(u[map](x => x), u)
identity(maybeEquals, Maybe.Just(1))

and imo the last one is better, instead of getting some object and extracting left, right parts you give a function and get the result (and also the eq function you pass does not have to return bool you can return promise for example it's up to user, so this is not "limiting" us)

@rpominov
Copy link
Member

This refactoring will be a breaking change strictly speaking though.

@safareli
Copy link
Member Author

yeh :/ that's true.

What if we state in readme that we might have some braking changes in laws, but still do patch releases if there is no change in actual spec.

@rjmk
Copy link
Contributor

rjmk commented Oct 26, 2016

Automated tools don't read READMEs. I think we need to stick to incrementing the major version for a breaking change

@rpominov
Copy link
Member

Let's move this discussion to #195

@safareli
Copy link
Member Author

despite the #195 can we merge this PR?

@davidchambers davidchambers merged commit c058124 into fantasyland:master Oct 27, 2016
@safareli safareli deleted the fix-laws branch October 27, 2016 13:56
rjmk added a commit to rjmk/fantasy-land that referenced this pull request Nov 12, 2016
* 'master' of github.com:fantasyland/fantasy-land: (29 commits)
  Version 2.1.0
  Add Alt, Plus and Alternative specs (fantasyland#197)
  Use uppercase letters for Type representatives in laws (fantasyland#196)
  Fix id_test and argument order in laws (fantasyland#193)
  Version 2.0.0
  Another go at updating dependencies (fantasyland#192)
  release: integrate xyz (fantasyland#191)
  test: remove unnecessary lambdas (fantasyland#190)
  require static methods to be defined on type representatives (fantasyland#180)
  lint: integrate ESLint (fantasyland#189)
  Enforce parametricity (fantasyland#184)
  readme: tweak signatures to indicate that methods are not curried (fantasyland#183)
  Fix reduce signature to not use currying (fantasyland#182)
  Link to dependent specifications (fantasyland#178)
  Add Fluture to the list of implementations (fantasyland#175)
  laws/functor: fix composition (fantasyland#173)
  laws/monad: fix leftIdentity (fantasyland#171)
  Minor version bump
  bower: add bower.json (fantasyland#159)
  Fix chainRec signature to not use currying (fantasyland#167)
  ...
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

5 participants