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

Add curried functions #37

Merged
merged 6 commits into from Oct 8, 2016

Conversation

Projects
None yet
3 participants
@boris-marinov
Contributor

boris-marinov commented Sep 7, 2016

This is Part I of the control.monad package because it became somewhat big (and because I am very late).

The PR includes all functions from the curried.js file of the prev library where:

  • I did not put explicit assertions for whether the object supports the method or whether the argument is of the correct type since we perform those at the datatypes themselves so it seemed wasteful to include them here also.
  • They are not auto-curried, as prescribed in the original design document for the new version.
  • I feel like the args of the concat function were not in the correct order in the original lib, so instead of (a) => (b) => a.concat(b);, it should be (b) => (a) => a.concat(b); (in the same way that map is (f, a)=>... ). What do you think?

The PR also includes:

  • Some really minor updates of the Maybe datatype.
  • A custom JSVerify environment which allows you to generate values from Folktale datatypes, using the JSVerify DSL.

This one, is very fresh but it seems to work perfectly. So for example if you have the test/environment.es6 underenv, you can write:

property('chain', 'json -> monad json', 'monad json', env, function(mf, ma) {

and mf will be a function that accepts a JSON value and returns a folktale monad (data.either or data.maybe) that contains a JSON value. ma will be a JSON value contained in the same monad.

As usual all comments are welcome!

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Sep 7, 2016

Member

This looks good, the JSVerify environment is pretty nice :)

Can you move the curried functions to the core/fantasy-land/curried module instead though? We'll need a function for each method in Fantasy Land now that they're moving to prefixed names (see #39). So it'd be cool if each function were like this:

const warnDeprecation = require('folktale/helpers/warnDeprecation');
const deprecatedFL = (name, result) =>{
  warnDeprecation(`Type.${name}() is being deprecated in favour of Type['fantasy-land/${name}']().

Your data structure is using the old-style fantasy-land methods, and these won't be supported in Folktale 3`);
  return result;
}

const map = (fn) => (type) =>
  type['fantasy-land/map'] ? type['fantasy-land/map'](fn)
: deprecatedFL('map', type.map(fn))

EDIT: I forgot to mention, but swapping the argument order for concat/equals/etc is a good idea (though it doesn't matter for equals). I've documented all the types here: https://github.com/origamitower/folktale/blob/master/ROADMAP.hs#L173-L234, but feel free to suggest something different if you think it's more useful :)

Member

robotlolita commented Sep 7, 2016

This looks good, the JSVerify environment is pretty nice :)

Can you move the curried functions to the core/fantasy-land/curried module instead though? We'll need a function for each method in Fantasy Land now that they're moving to prefixed names (see #39). So it'd be cool if each function were like this:

const warnDeprecation = require('folktale/helpers/warnDeprecation');
const deprecatedFL = (name, result) =>{
  warnDeprecation(`Type.${name}() is being deprecated in favour of Type['fantasy-land/${name}']().

Your data structure is using the old-style fantasy-land methods, and these won't be supported in Folktale 3`);
  return result;
}

const map = (fn) => (type) =>
  type['fantasy-land/map'] ? type['fantasy-land/map'](fn)
: deprecatedFL('map', type.map(fn))

EDIT: I forgot to mention, but swapping the argument order for concat/equals/etc is a good idea (though it doesn't matter for equals). I've documented all the types here: https://github.com/origamitower/folktale/blob/master/ROADMAP.hs#L173-L234, but feel free to suggest something different if you think it's more useful :)

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Sep 8, 2016

Contributor

OK, I am also seeing that there are more functions to be implemented.

Contributor

boris-marinov commented Sep 8, 2016

OK, I am also seeing that there are more functions to be implemented.

feat: various
Updated the environment to feature other types.

Added 'equals' and 'bimap'

Moved the module to fantasy-land/curried
@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Sep 10, 2016

Contributor

Added equals and bimap. Will leave the other methods for later, because currently we don't have types to test them with.

Contributor

boris-marinov commented Sep 10, 2016

Added equals and bimap. Will leave the other methods for later, because currently we don't have types to test them with.

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Sep 20, 2016

Contributor

I added the deprecation warnings.
Also, functions now throw exceptions when objects that do not support the corresponding methods are passed.

Note that we are using an old version of fantasy-land, that does not support bimap, this is why I haven't updated this function.

I reviewed the typedocs. On reduce, maybe the first two arguments (the function and the accumulator) should go together, at least I can't think of a usecase when they don't.

Contributor

boris-marinov commented Sep 20, 2016

I added the deprecation warnings.
Also, functions now throw exceptions when objects that do not support the corresponding methods are passed.

Note that we are using an old version of fantasy-land, that does not support bimap, this is why I haven't updated this function.

I reviewed the typedocs. On reduce, maybe the first two arguments (the function and the accumulator) should go together, at least I can't think of a usecase when they don't.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Sep 24, 2016

Member

I think we can use curry for these functions, since we'll also have non-curried variants of them. That way people can provide more than one argument to the curried reduce at a time. The new curry operation only unrolls application when the result is a curried function, too, so it doesn't have a problem with things like array.map(map(f)) anymore.

Member

robotlolita commented Sep 24, 2016

I think we can use curry for these functions, since we'll also have non-curried variants of them. That way people can provide more than one argument to the curried reduce at a time. The new curry operation only unrolls application when the result is a curried function, too, so it doesn't have a problem with things like array.map(map(f)) anymore.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Sep 24, 2016

Member

Ah, as for the method names, the idea is to just use the literals instead of depending on the fantasy-land package. So bimap would be like:

const bimap = 'fantasy-land/bimap';

module.exports = (f, g) => (a) => 
  typeof a[bimap] === 'function' ?  a[bimap](f, g)
  typeof a.bimap  === 'function' ?  a.bimap(f, g)
: /*otherwise*/                     unsupported(a);

The unsupported branch is a pretty nice thing there :3

Member

robotlolita commented Sep 24, 2016

Ah, as for the method names, the idea is to just use the literals instead of depending on the fantasy-land package. So bimap would be like:

const bimap = 'fantasy-land/bimap';

module.exports = (f, g) => (a) => 
  typeof a[bimap] === 'function' ?  a[bimap](f, g)
  typeof a.bimap  === 'function' ?  a.bimap(f, g)
: /*otherwise*/                     unsupported(a);

The unsupported branch is a pretty nice thing there :3

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Sep 29, 2016

Contributor

Using curry will make the whole module much easier to maintain.

I think defining the curried functions manually may prevent some errors, that may occur in cases when two or more arguments always go together, like in bimap and reduce, but most of the functions aren't such, so I agree that it's better to use curry.

Just to be clear, what you want is

  1. Uncurry all these functions (if this is a word :)).
  2. Move them in core.fantasyland.
  3. Make a curried module which reexports all the functions, with curry, applied on them (possibly we can automate the last step).
Contributor

boris-marinov commented Sep 29, 2016

Using curry will make the whole module much easier to maintain.

I think defining the curried functions manually may prevent some errors, that may occur in cases when two or more arguments always go together, like in bimap and reduce, but most of the functions aren't such, so I agree that it's better to use curry.

Just to be clear, what you want is

  1. Uncurry all these functions (if this is a word :)).
  2. Move them in core.fantasyland.
  3. Make a curried module which reexports all the functions, with curry, applied on them (possibly we can automate the last step).
@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Sep 29, 2016

Contributor

Out of curiosity, why do you want to avoid depending on Fantasy Land for the method names?

Contributor

boris-marinov commented Sep 29, 2016

Out of curiosity, why do you want to avoid depending on Fantasy Land for the method names?

@safareli

This comment has been minimized.

Show comment
Hide comment
@safareli

safareli Sep 29, 2016

Maybe because of FL's dependencies

safareli commented Sep 29, 2016

Maybe because of FL's dependencies

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Oct 2, 2016

Member

@boris-marinov yup, pretty much that.

As for the dependency in the fantasy-land module: It has to be installed as a peer dependency, and so the user gets to decide which version of the spec would be used. This would be a-okay if all fantasy-land implementations used the fantasy-land module as a peer dependency, because that way all of them would define the methods on (and use the methods) at with the right name.

Since most implementations don't use the library (the old Folktale modules don't, for example) you have to account for all different versions of the fantasy-land spec anyway, and the fantasy-land module doesn't let you do that (if it publishes a 2.x version with different names, then libraries without it in 1.x wouldn't be supported).

The other problem is that we place the burden on the user to decide which version of the fantasy-land module to pick, since it can't be installed as a regular dependency.

So, mostly I don't see the advantage in bringing that dependency when spec changes are not frequent, and we'd want to support older specs in any case.

edit: (also, the package.json doesn't define fantasy-land as a peer dependency right now, which leads to worse problems, like having two incompatible versions of fantasy-land used by different libraries)

Member

robotlolita commented Oct 2, 2016

@boris-marinov yup, pretty much that.

As for the dependency in the fantasy-land module: It has to be installed as a peer dependency, and so the user gets to decide which version of the spec would be used. This would be a-okay if all fantasy-land implementations used the fantasy-land module as a peer dependency, because that way all of them would define the methods on (and use the methods) at with the right name.

Since most implementations don't use the library (the old Folktale modules don't, for example) you have to account for all different versions of the fantasy-land spec anyway, and the fantasy-land module doesn't let you do that (if it publishes a 2.x version with different names, then libraries without it in 1.x wouldn't be supported).

The other problem is that we place the burden on the user to decide which version of the fantasy-land module to pick, since it can't be installed as a regular dependency.

So, mostly I don't see the advantage in bringing that dependency when spec changes are not frequent, and we'd want to support older specs in any case.

edit: (also, the package.json doesn't define fantasy-land as a peer dependency right now, which leads to worse problems, like having two incompatible versions of fantasy-land used by different libraries)

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Oct 4, 2016

Contributor

True.

The best bet is to have an (implicit) peer dependency on one hand and default values in case the peer dependency is not defined, on another. SO says that we can do:

  try {
      require.resolve("fantasy-land");
  } catch(e) {
      module.exports = {
         map: 'fantasy-land/map'
         ...
      }
  }
  module.exports = require('fantasy-land')
}

but I haven't tested it.

Also, since we use the method names in at least two occasions (at the place where each type is defined and in core.fantasyLand), I think it makes sense to move them at a separate file in all cases.

Otherwise I refactored the FL functions to use curry. Note that I pass function.length as arity, let me know if you disagree with that.

Contributor

boris-marinov commented Oct 4, 2016

True.

The best bet is to have an (implicit) peer dependency on one hand and default values in case the peer dependency is not defined, on another. SO says that we can do:

  try {
      require.resolve("fantasy-land");
  } catch(e) {
      module.exports = {
         map: 'fantasy-land/map'
         ...
      }
  }
  module.exports = require('fantasy-land')
}

but I haven't tested it.

Also, since we use the method names in at least two occasions (at the place where each type is defined and in core.fantasyLand), I think it makes sense to move them at a separate file in all cases.

Otherwise I refactored the FL functions to use curry. Note that I pass function.length as arity, let me know if you disagree with that.

@safareli

This comment has been minimized.

Show comment
Hide comment
@safareli

safareli Oct 4, 2016

That implicit peer dependency idea looks nice 👍

safareli commented Oct 4, 2016

That implicit peer dependency idea looks nice 👍

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Oct 6, 2016

Member

Using function.length is fine here, since we know it's always going to have the right value in this case.

We can have a helper module exporting those. require.resolve() doesn't work with Browserify, but having a helper module export these names is a nice idea (in particular because we also avoid typos):

// helpers/fantasy-land.js
module.exports = {
    equals: 'fantasy-land/equals',
    concat: 'fantasy-land/concat',
    empty: 'fantasy-land/empty',
    map: 'fantasy-land/map',
    ap: 'fantasy-land/ap',
    of: 'fantasy-land/of',
    reduce: 'fantasy-land/reduce',
    traverse: 'fantasy-land/traverse',
    chain: 'fantasy-land/chain',
    chainRec: 'fantasy-land/chainRec',
    extend: 'fantasy-land/extend',
    extract: 'fantasy-land/extract',
    bimap: 'fantasy-land/bimap',
    promap: 'fantasy-land/promap'
}
// core/fantasy-land/ap.js
const { ap } = require('folktale/helpers/fantasy-land');

module.exports = (a, b) =>
  ...
Member

robotlolita commented Oct 6, 2016

Using function.length is fine here, since we know it's always going to have the right value in this case.

We can have a helper module exporting those. require.resolve() doesn't work with Browserify, but having a helper module export these names is a nice idea (in particular because we also avoid typos):

// helpers/fantasy-land.js
module.exports = {
    equals: 'fantasy-land/equals',
    concat: 'fantasy-land/concat',
    empty: 'fantasy-land/empty',
    map: 'fantasy-land/map',
    ap: 'fantasy-land/ap',
    of: 'fantasy-land/of',
    reduce: 'fantasy-land/reduce',
    traverse: 'fantasy-land/traverse',
    chain: 'fantasy-land/chain',
    chainRec: 'fantasy-land/chainRec',
    extend: 'fantasy-land/extend',
    extract: 'fantasy-land/extract',
    bimap: 'fantasy-land/bimap',
    promap: 'fantasy-land/promap'
}
// core/fantasy-land/ap.js
const { ap } = require('folktale/helpers/fantasy-land');

module.exports = (a, b) =>
  ...
@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Oct 7, 2016

Contributor

It will also enable us to use the Fantasy Land lib later, if appropriate.

Contributor

boris-marinov commented Oct 7, 2016

It will also enable us to use the Fantasy Land lib later, if appropriate.

@robotlolita robotlolita merged commit 160904d into master Oct 8, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@robotlolita robotlolita deleted the control.monad branch Oct 8, 2016

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Oct 8, 2016

Member

Thankies :)

Member

robotlolita commented Oct 8, 2016

Thankies :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment