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

Feature/filterable #485

Closed
wants to merge 48 commits into from
Closed

Feature/filterable #485

wants to merge 48 commits into from

Conversation

raveclassic
Copy link
Collaborator

This PR adds Compactable/Filterable/Witherable typeclasses (based on https://github.com/LiamGoodacre/purescript-filterable) as well as some implementations for Array/Option/Either/StrMap/Set.
Also Set now implements Witherable (Functor/Compactable/Filterable/Traversable/Foldable) (please, check it carefully, there's some API duplication, private though, because I don't get why Sets accept additional instances for map etc.).

There're also some helpers added, take a look.

@gcanti
Copy link
Owner

gcanti commented Jun 19, 2018

Thanks @raveclassic for this PR. I'm on the fence with this: when preparing the 1.0.0 release I removed Filterable and Witherable (a porting from purescript-filterable) because I didn't feel they were stable enough.

I think we should discuss such type classes one by one.

For example let's consider Compactable, at first sight looks like the only useful instance is for Array, which is catOptions.

Also compact and separate are equivalent (like traverse and sequence), so Compactable should contain either compact or separate, not both

export interface Compactable<F> {
  readonly URI: F
  readonly compact: <A>(fa: HKT<F, Option<A>>) => HKT<F, A>
  readonly separate: <A, B>(fa: HKT<F, Either<A, B>>) => Separated<HKT<F, A>, HKT<F, B>>
}

So is Compactable a good abstraction? Is it worth to add it to the core? I'm not sure and I'd like to see some compelling use cases.
Same for Filterable and Witherable.

Another option would be to publish a separate library, let's say fp-ts-filterable and see if it sticks.

/cc @sledorze how you feel about this? Do you know of equivalent abstractions in Scala (cats, scalaz)?

@raveclassic
Copy link
Collaborator Author

raveclassic commented Jun 19, 2018

@gcanti Thanks for response!
Meanwhile I added getCompact/getSeparate helpers with default implementations (same as compactDefault/separateDefault in purescript). So only one method should be defined and the other goes out of the box.

About Compactable - it's powerful enough to build everything up to Witherable using default implementations which I'd like to add further. And when there're some performance issues a perfomant version may be added. Compactible/Filterable/Witherable may also be useful with Trees/Sets etc.

Anyway I'd also like to discuss it first :)

@gcanti
Copy link
Owner

gcanti commented Jun 19, 2018

About Compactable - it's powerful enough to build everything up to Witherable

@raveclassic That's awesome! I didn't know that, thanks for explaining.

So Filterable and Witherable look useless, can we just add the Compactable type class?

// Compactable.ts

import { Applicative } from 'fp-ts/lib/Applicative'
import { Either } from 'fp-ts/lib/Either'
import { Functor } from 'fp-ts/lib/Functor'
import { HKT } from 'fp-ts/lib/HKT'
import { Traversable, traverse } from 'fp-ts/lib/Traversable'

export type Separated<A, B> = {
  readonly left: A
  readonly right: B
}

export interface Compactable<F> {
  readonly URI: F
  readonly separate: <A, B>(fab: HKT<F, Either<A, B>>) => Separated<HKT<F, A>, HKT<F, B>>
}

export function partitionMap<F>(
  F: Compactable<F> & Functor<F>
): <A, L, R>(fa: HKT<F, A>, f: (a: A) => Either<L, R>) => Separated<HKT<F, L>, HKT<F, R>> {
  return (fa, f) => F.separate(F.map(fa, f))
}

export function wilt<F, T>(
  F: Applicative<F>,
  T: Traversable<T> & Compactable<T>
): <A, L, R>(ta: HKT<T, A>, f: (a: A) => HKT<F, Either<L, R>>) => HKT<F, Separated<HKT<T, L>, HKT<T, R>>> {
  const traverseFT = traverse(F, T)
  return (ta, f) => F.map(traverseFT(ta, f), T.separate)
}

(overloadings omitted for clarity)

@raveclassic
Copy link
Collaborator Author

Yeah, that was my first attempt :) But very quickly I realized that such default implementation isn't performant enough for Array and Set. Even more they already have their own implementation which duplicates API. Looks like a place for Filterable to preserve avility to build higher-level abstractions on top of it with custom implementations which may be easily based on default implementations for quick prototyping (which I'm writing right now) or may be tuned for performance.
Same goes for Witherable.

I'm pushing my latest changes now (defaults in Filterable), take a look, what do you think? (I'm just porting purescript-filterable, some helpers may be removed)

@raveclassic
Copy link
Collaborator Author

raveclassic commented Jun 19, 2018

Well... Actually I don't like what's going on because such approach with defaults bloats API massively and brings in a lot of confusion from which method to start implementing Witherable. Need some time to think it over.

@gcanti
Copy link
Owner

gcanti commented Jun 19, 2018

Yeah, that was my first attempt

Not sure I understand why we can't just add Compactable plus some satellite functions like partitionMap and wilt.

If both Filterable and Witherable can be derived from Compactable what's the point of adding them?

Then every module can choose either to use the default implementation (of partitionMap or wilt) or a more performant one, like we can already do, for example, with Foldable and its satellite functions

@raveclassic
Copy link
Collaborator Author

@gcanti But there's no way to require some data structure to implement filterMap/partitionMap (implement Filterable) performantly. Arrays, Sets, Trees and others all implement the same interface which is so tempting to be moved to abstraction.
But honestly I can't think of any better usecase except of "generalization for the sake of generalization", really :)

Ok, I'll clean up this PR and leave only useful things in Compactable module as well as some improvements for existing code. Let's take some time and see how it goes.

@raveclassic
Copy link
Collaborator Author

After some thoughts it still feels intuitively incorrect to me to move partitionMap and wilt to Compactable and to drop Filterable and Witherable. Also what about filterMap/wither? Could you please explain why don't you want to include full abstraction each part of which has its own isolated responsibility? (I'm talking about interfaces now, not implementations)
Array, Set, Tree, Option, Either, Identity, Const (and I think some others) implement all these interfaces. If we have a Filterable typeclass then Array's uniq can be generalized, otherwise you end up specifying both Functor<F> & Compactable<F> in its signature which is essentially the same as Filterable<F> but more verbose and doesn't accept optimized implementation. refine, span, remove and many others can be generalized the same way.

What about implementations. I do agree that forcing to define all methods (compact, separate, partition, filter, partitionMap, filterMap, wilt and wither) is pointless but I don't understand why can't we just put default implementations to the instance? Like const array: Compactable<URI> = { compact, separate: getSeparateFromCompact(compact) }. And leave the possibility to add more performant versions later? All these functions can be written from scratch to provide max performance but still there's a room for quick prototyping when writing custom ADT.

@gcanti
Copy link
Owner

gcanti commented Jun 20, 2018

There are several topics involved, let's try to discuss one by one

Performance

I don't understand why can't we just put default implementations to the instance?

We could do this, but what about Traversable? Should we add sequence to its definition?
What about Foldable? Should we add foldaMap and foldr? I'm not against the idea but since it would be a breaking change we must discuss it further. In the meanwhile I'd stick to the current convention of defining only a minimal interface

New type classes

otherwise you end up specifying both Functor & Compactable in its signature which is essentially the same as Filterable

You are right, Filterable makes sense.

Maybe I just need a plan to get the big picture:

  • Compactable
    • minimal definition: separate (or compact)
    • instances
      • Array
      • others?
    • satellite functions
      • compact
      • others?
  • Filterable
    • minimal definition: just a type alias of Compactable & Functor
    • instances
      • Array
      • others?
    • satellite functions
      • partitionMap?
      • partition?
      • filterMap?
      • filter?
      • others?
  • Witherable
    • minimal definition: just a type alias of Compactable & Traversable
    • instances
      • Array
      • others?
    • satellite functions
      • wilt?
      • wither?
      • others?

@raveclassic
Copy link
Collaborator Author

raveclassic commented Jun 20, 2018

I'm not against the idea but since it would be a breaking change we must discuss it further.

IMO this makes sense in the long-term. As fp-ts continuosly evolves gaining new features it seems correct to provide wider API and to tune performance. However we could omit these typeclasses for 1.6.x versions to avoid breaking changes and update them later.

Looks like a plan. I'd also add the following:

  • Compactable
    • instances
      • Array
      • Set
      • StrMap
      • Option
      • Identity
      • Const
      • Tree
      • Pair
      • Either ? (current implementation requires Monoid for empty left side, is it ok?)
      • Validation ? (same as Either)
      • Tuple ? (same as Either)
    • helpers (maybe some other names)
      • getCompactFromSeparate
      • getSeparateFromCompact
      • getCompactFromFilterMap ??? (we need to stop somewhere :) )
  • Filterable
    • minimal definition: shouldn't all partition/filter/partitionMap/filterMap be included?
    • instances: all the above
    • helpers
      • port of purescript code ? may be extended
  • Witherable - same as above

@raveclassic
Copy link
Collaborator Author

raveclassic commented Jun 20, 2018

Also I'd move all generalizable functions from Array to Filterable (Yeah, there's still a lot of work here)

@gcanti
Copy link
Owner

gcanti commented Jun 20, 2018

IMO this makes sense in the long-term
shouldn't all partition/filter/partitionMap/filterMap be included?

Ok, so you want to start from Compactable / Filterable / Witherable with this new style, right?
Then if it works well we can retrofit this new style to Foldable / Traversable etc... (likely in a future 2.0 version though)

for 1.6.x versions

The next version will be 1.7.0 (current active milestone)

I'd also add the following...

Overall looks good. Could you please split this plan in chunks, sending multiple PRs?

Let's start from Compactable (definition + helpers) and then we can think about its instances (Array, Set, etc...)

Filterable... port of purescript code ? may be extended
Also I'd move all generalizable functions from Array to Filterable

Not sure what you mean, let's discuss this later though, one thing at a time.

@raveclassic
Copy link
Collaborator Author

Ok, so you want to start from Compactable / Filterable / Witherable with this new style, right?
Then if it works well we can retrofit this new style to Foldable / Traversable etc... (likely in a future 2.0 version though)

Yeah, exactly

The next version will be 1.7.0 (current active milestone)

Should I change all @since 1.6.3 tags to @since 1.7.0 in this PR then?

Overall looks good. Could you please split this plan in chunks, sending multiple PRs?

Sure, I've already split branches, going to open PR with Compactable soon.

This was referenced Jun 20, 2018
@raveclassic
Copy link
Collaborator Author

This PR will be split into parts, closing.

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.

2 participants