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

Setoid derivation #10

Merged
merged 7 commits into from May 8, 2016

Conversation

Projects
None yet
2 participants
@boris-marinov
Contributor

boris-marinov commented Apr 29, 2016

We haven't discussed derivations much but looking at the code I assume that this is what you had in mind.

This derivation adds an equals method to each variant. The equals method works by comparing the types of the two objects first and then checking all their properties for equality using === or equals if supported.

We will probably want to export the derivation itself, so it is available externally. I haven't done it yet because I am not sure where should we put the build-in derivations.

We can add them to the data function:

const type = data({
 ...
}).derive(data.setoid) 

or change the structure of the export:

const {data, setoid} = require('adt')
const type = data({
 ...
}).derive(setoid) 

or include some syntactic sugar for applying the default derivations (they are the ones which are used most often so I think it is worth it):

const type = data({
 ...
}).setoid() 

I also placed the assertType function to a separate module, like we discussed.

One small issue that is also related to the ADT discussion: We don't specify the name of the type we are creating (e.g. we don't have the name "Either", "Maybe" etc. as a string).
In this case we need the type for displaying it in the the error messages, so it's not a show-stopper but in other cases it seems that it is.
I added a generic label in the assertType function to deal with this particular instance.

Cheers.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Apr 30, 2016

Member

I think changing the structure of the export would be the best here. So moving the current data implementation to a core.js module, and having an index.js that re-exports all things below, like in Maybe or Either, sounds like a good thing.

For the Setoid derivation in particular, instead of using === for non-setoids, it'd be better to parameterise the equality, so something like:

module.exports = (equals) => {
  const isEqual = (a, b) =>
    isSetoid(a) && isSetoid(b)?  a.equals(b)
    : /* otherwise */            equals(a, b)

  return (variant, adt) => {
    /* ... */ 
  }
}

'sides that, this is pretty much what I had in mind for derivations, indeed.

I hadn't thought about the name of the type before, but that might be important for deriving serialisation automatically.

Member

robotlolita commented Apr 30, 2016

I think changing the structure of the export would be the best here. So moving the current data implementation to a core.js module, and having an index.js that re-exports all things below, like in Maybe or Either, sounds like a good thing.

For the Setoid derivation in particular, instead of using === for non-setoids, it'd be better to parameterise the equality, so something like:

module.exports = (equals) => {
  const isEqual = (a, b) =>
    isSetoid(a) && isSetoid(b)?  a.equals(b)
    : /* otherwise */            equals(a, b)

  return (variant, adt) => {
    /* ... */ 
  }
}

'sides that, this is pretty much what I had in mind for derivations, indeed.

I hadn't thought about the name of the type before, but that might be important for deriving serialisation automatically.

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov May 3, 2016

Contributor
  • Rearranged the module structure as we discussed
  • Setoid now accepts an equal function (also added a default implementation).
  • The linter was ignoring the ADT module - turned on the linter and fixed the problems.
Contributor

boris-marinov commented May 3, 2016

  • Rearranged the module structure as we discussed
  • Setoid now accepts an equal function (also added a default implementation).
  • The linter was ignoring the ADT module - turned on the linter and fixed the problems.
@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita May 6, 2016

Member

Okay, it looks good to me. We can merge it after #11 is reviewed, and we have a final API for the ADT module :)

Member

robotlolita commented May 6, 2016

Okay, it looks good to me. We can merge it after #11 is reviewed, and we have a final API for the ADT module :)

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov May 6, 2016

Contributor

Super!
I can't wait to resume my work on Folktale.

One thing I thought about when I was reviewing the ADT docs: Currently the function that we export under the name setoid is a function which accepts an equal definition (or undefined for using ===), and returns a derivation:

const Either = data({
    Left:  ['value'],
    Right: ['value']
}).derive(setoid());

Some other derivations will not need to be parametrised and can possibly be exported directly.

const List = data('List', {
   Nil:  () => {},
   Cons: (value, rest) => ({ value, rest })
}).derive(ToJSON);

I was thinking that this may lead to confusion as to which exports are functions that return a derivation and which are just derivations. What are your thoughts on this?

Contributor

boris-marinov commented May 6, 2016

Super!
I can't wait to resume my work on Folktale.

One thing I thought about when I was reviewing the ADT docs: Currently the function that we export under the name setoid is a function which accepts an equal definition (or undefined for using ===), and returns a derivation:

const Either = data({
    Left:  ['value'],
    Right: ['value']
}).derive(setoid());

Some other derivations will not need to be parametrised and can possibly be exported directly.

const List = data('List', {
   Nil:  () => {},
   Cons: (value, rest) => ({ value, rest })
}).derive(ToJSON);

I was thinking that this may lead to confusion as to which exports are functions that return a derivation and which are just derivations. What are your thoughts on this?

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita May 6, 2016

Member

Hm, people'd need to look at the type of the exported function to know how to use it, but we could also have different constructors, like what's used in mapEntries (https://github.com/origamitower/folktale/blob/master/src/core/object/map-entries.js):

data(...).derive(Setoid) // uses the default equality
data(...).derive(Setoid.withEquality(equalsFn)) // uses provided equality

Or we could require all exports to be functions returning derivations for consistency. Particularly I'd lean more towards the different constructors approach, which is more flexible if it makes sense for the derivation to be configured in different ways.

Member

robotlolita commented May 6, 2016

Hm, people'd need to look at the type of the exported function to know how to use it, but we could also have different constructors, like what's used in mapEntries (https://github.com/origamitower/folktale/blob/master/src/core/object/map-entries.js):

data(...).derive(Setoid) // uses the default equality
data(...).derive(Setoid.withEquality(equalsFn)) // uses provided equality

Or we could require all exports to be functions returning derivations for consistency. Particularly I'd lean more towards the different constructors approach, which is more flexible if it makes sense for the derivation to be configured in different ways.

boris-marinov added some commits May 7, 2016

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov May 8, 2016

Contributor

Good idea. I am glad that I brought that up.

Implemented.

Quick question: is there a reason for not using arrow functions in the tests?

Contributor

boris-marinov commented May 8, 2016

Good idea. I am glad that I brought that up.

Implemented.

Quick question: is there a reason for not using arrow functions in the tests?

@robotlolita robotlolita merged commit cd126c4 into master May 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 Setoid-derivation branch May 8, 2016

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