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

Fantasy Land method aliases #257

Closed
davidchambers opened this issue Sep 23, 2016 · 9 comments
Closed

Fantasy Land method aliases #257

davidchambers opened this issue Sep 23, 2016 · 9 comments

Comments

@davidchambers
Copy link
Member

There are several related questions which must be answered before we can merge #216. Discussing these in a dedicated thread seems wise, as that pull request is very large.

Currently we provide Maybe and Either types compatible with the Fantasy Land spec prior to the merging of fantasyland/fantasy-land#146. In #216 we're adding support for the prefixed method names. Should we continue to expose methods via the unprefixed names? Concretely, should we provide Maybe#map and Maybe#fantasy-land/map or just the latter?

Reasons to alias (:alien:):

Reasons not to alias (:no_entry_sign: :alien:):

  • it would violate The Zen of Python (python -c 'import this' | grep 'only one');
  • it would increase Sanctuary's surface area (more to document and test); and
  • method chaining is at odds with Sanctuary's emphasis on function composition.

If we decide to include these aliases, there's a second question to answer: should we also provide Maybe.empty, Maybe.of, and Either.of? These are already aliases (for () => Nothing, Just, and Right respectively). There's value, of course, in providing the prefixed equivalents to facilitate the creation of polymorphic functions, but if one is dealing with the Maybe type, say, I see no reason to write Maybe.empty() rather than Nothing. Am I missing anything?

The final question is what to do with Maybe#filter. It's currently defined for compatibility with R.filter and R.reject. My plan, though, is to define filter in sanctuary-type-classes, since it can be derived from other FL functions:

//# filter :: (Monad m, Monoid m a) => ((a -> Boolean), m a) -> m a
var filter = function(pred, m) {
  return chain(function(x) { return pred(x) ? of(m, x) : empty(m); }, m);
};

Thanks, @joneshf, for sharing this approach way back in #10!

With Z.filter in place, #216 could easily add S.filter and S.reject. At that point Maybe#filter would only be useful for Ramda compatibility. Is that important, do you think?

I have views on these matters, but I'd love to hear from @safareli and @Bradcomp (and others) before weighing in.

@safareli
Copy link
Member

safareli commented Sep 23, 2016

it would increase Sanctuary's surface area (more to document and test); and

How I see this is that we could just document normal non namespaced functions and state somewhere which FL interfaces the structure conforms to. About tests if a.map === a[fl.map] and if one is tested then other one is tested too as thay are same so why do we need to write more tests. or at least we would not need to write too many tests. maybe just check if structure implements some FL method it to also have non namespace alias. So I dont see this as a big issue.

method chaining is at odds with Sanctuary's emphasis on function composition.

I understand that Sanctuary emphasis function composition, but I don't think we should make it painful to do chaining. Also even though composition is great, Sometimes it's easier to add ... .map(f) to so(m(e(big),ex),pre,sion) then wrapping it in map(f, ...) when you the expression is not in composition. Also as chaining is pure in this case it's still good think and I don't see why should we make it hard to do.

it would violate The Zen of Python (python -c 'import this' | grep 'only one');

That's a good point but if we see fantasy-land/methods as private interface which is used to communicate between fantasy-land structures, then there are no multiple ways to do one think.

If in a code we are working with some specific version of Maybe and want to do map over it, then we would just just write a.map(f)

If we are writing some generic function like filter which requires it's arguments to conform to some FL interface then in that function we should definitely use namespaced methods.


Also to note I'm not saying that for example S.lift should use .map instead I think it should work with [fl.map] only


Related: my comment on same discussion in folktale#39

@davidchambers
Copy link
Member Author

About tests if a.map === a[fl.map] and if one is tested then other one is tested too as thay are same so why do we need to write more tests.

Absolutely. In fact, I have eq(x[methodName], x[FantasyLand[methodName]]); in the test suite right now, so testing all the aliases requires just one line of code. The documentation is more of an issue.

That's a good point but if we see fantasy-land/methods as private interface which is used to communicate between fantasy-land structures, then there are no multiple ways to do one think.

I agree that we should consider the prefixed methods private. There are still two ways to do it, though: maybe.map(f) and S.map(f, maybe). I worry that documenting the former will make new Sanctuary users less likely to discover S.map, which in turn will result in fewer functional pipelines.

It's worth noting that I was able to improve performance significantly in #216 by removing type checking from methods. Since Ramda does not do type checking, methods such as Either#chain needed to perform type checking in order to catch misuses of R.chain (this check caught a type error in a package with 100% coverage when I upgraded its Sanctuary dependency to a version with type-checked methods). Sanctuary will soon provide its own Fantasy Land functions which can take on the responsibility of type checking. Sanctuary users who use methods directly will not receive helpful error messages when they err.

I understand that Sanctuary emphasis function composition, but I don't think we should make it painful to do chaining.

I agree. I'm happy for the aliases to exist; I'm just not comfortable documenting their existence. If advanced users want to use Maybe#map, that's fine with me. If users with existing code don't want to switch to S.map when upgrading, that's fine with me too. But I want to encourage new Sanctuary users to reach for S.map.

If we are writing some generic function like filter which requires it's arguments to conform to some FL interface then in that function we should definitely use namespaced methods.

Absolutely. The point is, though, that we can have S.filter and S.reject without defining Maybe#filter, but if we want values of the Maybe type to work with R.filter and R.reject we need to define Maybe#filter (since the Ramda functions dispatch to filter).

@safareli
Copy link
Member

Sanctuary will soon provide its own Fantasy Land functions which can take on the responsibility of type checking. Sanctuary users who use methods directly will not receive helpful error messages when they err.

so Maybe#map and Maybe#fantasy-land/map would not be typechecked?

@davidchambers
Copy link
Member Author

so Maybe#map and Maybe#fantasy-land/map would not be typechecked?

That's the current situation in #216, yes. Paying the cost of type checking twice—once in S.map and once in Maybe#fantasy-land/map—doesn't seem fair.

Do you think the methods should be type checked? I'm not convinced that it was correct to remove the type checking.

@Bradcomp
Copy link
Member

Aliasing

Pros

to maintain Ramda compatibility until ramda/ramda#1900 is merged;

👍

to support method chaining.

👎

Cons

method chaining is at odds with Sanctuary's emphasis on function composition.
it would violate The Zen of Python

👌

I recently started a new project and was evaluating which libraries to use for functional tools and types. I like Sanctuary because it is much more opinionated than Ramda in various areas (type safety and variadic functions to name two). For that reason I am all for deprecating the non-prefaced versions of the methods. Sanctuary makes it easy to write code in a certain way, and helps me out if I make a mistake in a way that Ramda doesn't. Especially once Sanctuary includes functions for all the FL methods, I see no reason to use the non-prefixed methods in practice. There are plenty of other libraries for ADTs that will support the non-prefixed versions and are good choices if you want to use method chaining.

On the other hand, I don't support removing the non-prefixed versions for a little while. It would be nice to not break compatibility with existing code until libraries are caught up with FL 1.0. I would argue that both the prefaced and non-prefaced versions should be considered a private interface.

Of

It would be nice to have an S.of function (does it already exist?) that takes a type as a parameter.
of :: Applicative m => m -> a -> m a

Filter and Reject

I am all for creating Sanctuary versions of Ramda functions where it makes sense to do so, and I do think this is a case where it makes sense to do so.

Typechecked methods

Given what I've said above, I think it's fair to not typecheck the methods. However, the one down side is then if I write a generic function to operate on a ADT it will lose out on the type checking. On the other hand, I could always define the function using sanctary-def to get that type checking back, so I think it's a wash.

@davidchambers
Copy link
Member Author

Thank you both for your thoughtful feedback.

I intend to do the following:

  • Add prefixes to the documentation of the methods. We'll document Maybe#fantasy-land/map, for example, rather than Maybe#map.
  • Update the code examples to use S functions. The doctests for Maybe#fantasy-land/map, for example, will feature S.map rather than direct method invocations.
  • Leave the method aliases. These will still be verified by the test suite, but will no longer be documented. We may decide to remove these at some point in the future.
  • Remove Maybe.empty, Maybe.of, and Either.of. These simply aren't necessary.
  • Remove Maybe#filter. fantasy land: add Fantasy Land functions #216 will add S.filter and S.reject so I don't think maintaining compatibility with the Ramda equivalents is important.

It would be nice to have an S.of function (does it already exist?) that takes a type as a parameter.

It doesn't exist yet, but we can add it in #216 once we agree on the appropriate type of Z.of in sanctuary-js/sanctuary-type-classes#3.

@davidchambers
Copy link
Member Author

I'm reconsidering this issue.

My long-term goal is to move the Maybe and Either types to their own packages. These packages will only provide the data constructors and the various fantasy-land/-prefixed methods. They will be compatible with Sanctuary and sanctuary-type-classes (and Ramda if it is updated to support version 2 of the FL spec). I do not intend to provide unprefixed methods. Those who prefer method chaining to function composition are better served by Folktale, which is to Scala as Sanctuary is to Haskell.

Given this goal, what is the best transition? Initially I had thought it wise to keep the unprefixed methods around for the next six months, say, to making upgrading to sanctuary@0.12.x less onerous. I'm now questioning this logic. sequence has been replaced with fantasy-land/traverse. Should we keep the sequence methods around for some time? The type of fantasy-land/ap differs from that of ap. Should we keep both versions of the method? What about new methods? Should we provide alt in addition to fantasy-land/alt? alt is not required for backwards compatibility, but providing some aliases but not others for historical reasons would be confusing for new users.

I'm now in favour of removing all the aliases from #232. This will mean code which invokes Sanctuary methods directly will need to be updated when upgrading to v0.12.0, but this cost will need to be paid at some point regardless.

@davidchambers davidchambers reopened this Nov 15, 2016
@Bradcomp
Copy link
Member

I think given the work you've done on sanctuary-type-classes, it makes a lot of sense to remove the aliases. By using the provided functions in conjunction with the data types, one does not have to worry about the types and functions falling out of sync.

If one doesn't want to use the functions provided, they can make sure to update their calling code, or just not update Sanctuary. Eventually that would have to be the case anyway, might as well do it once you have the functions available to manage the transition.

@davidchambers
Copy link
Member Author

Thanks for the additional feedback, folks. :)

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

No branches or pull requests

3 participants