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

Conversions #24

Closed
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@boris-marinov
Contributor

boris-marinov commented Jul 10, 2016

Includes

  1. A Conversions module which consists of six functions of type X -> Y which convert each of the three types Maybe, Either and Validation to all other types. The module is private (it is not exported in data). The functions for converting a Maybe to an Either or Validation accept as a second parameter the error value which is to be put in the Either/Validation in case the first parameter is Nothing.
  2. Aliases to all of the module's functions as part of the type definitions (so Either.fromValidation is an alias to validationToEither).
  3. Methods for converting an instance to another one which call the above function (so Left(1).toEither() is leftToEither(Left(1))). This introduces a circular dependency which is resolved by requiring the helper functions after the type definition is exported in the core.js files for all datatypes.
@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Jul 11, 2016

Member

Ah, that's neat 👍

Instead of putting the requires at the bottom and relying on Node's caching (which I'm not too fond of), we can just move those requires inside of the function. Like:

Maybe.toEither = function(...args) {
  return require('folktale/data/conversions/maybe-to-either')(this, ...args);
};

Since these are not resolved eagerly, mutual recursion just works. Caching solves the loading problem if people call it many times.

Member

robotlolita commented Jul 11, 2016

Ah, that's neat 👍

Instead of putting the requires at the bottom and relying on Node's caching (which I'm not too fond of), we can just move those requires inside of the function. Like:

Maybe.toEither = function(...args) {
  return require('folktale/data/conversions/maybe-to-either')(this, ...args);
};

Since these are not resolved eagerly, mutual recursion just works. Caching solves the loading problem if people call it many times.

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Jul 12, 2016

Contributor

Done.

Contributor

boris-marinov commented Jul 12, 2016

Done.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Jul 18, 2016

Member

LGTM. Could you move fromNullable to data/conversions and add one for Either as well?

Member

robotlolita commented Jul 18, 2016

LGTM. Could you move fromNullable to data/conversions and add one for Either as well?

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Jul 18, 2016

Contributor

Good idea.

Contributor

boris-marinov commented Jul 18, 2016

Good idea.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Jul 18, 2016

Member

Thanks :)

I suppose this has to wait Validation to be merged now.

Member

robotlolita commented Jul 18, 2016

Thanks :)

I suppose this has to wait Validation to be merged now.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Jul 22, 2016

Member

Could you rebase this on master?

Member

robotlolita commented Jul 22, 2016

Could you rebase this on master?

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Jul 22, 2016

Contributor

oops I think I messed it up.
I will pull from master and try to rebase again when I have time.
sorry.

Contributor

boris-marinov commented Jul 22, 2016

oops I think I messed it up.
I will pull from master and try to rebase again when I have time.
sorry.

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov
Contributor

boris-marinov commented Jul 25, 2016

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Jul 27, 2016

Member

Thanks :)

Member

robotlolita commented Jul 27, 2016

Thanks :)

@robotlolita robotlolita deleted the conversions branch Sep 1, 2016

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