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

NonEmptyArray and ReadonlyNonEmptyArray are missing functions from Array and ReadonlyArray #1420

Closed
jleider opened this issue Mar 3, 2021 · 8 comments
Milestone

Comments

@jleider
Copy link
Contributor

jleider commented Mar 3, 2021

🐛 Bug report

There are a number of functions that exist within Array and ReadonlyArray that are absent from NonEmptyArray and ReadonlyNonEmptyArray.

Some of them may not be applicable to their NonEmpty variants or may return an Option<NonEmptyArray> or a normal Array from a NonEmptyArray such as with filter style functions.

At this point its a design decision and I'm not sure if its better to have these filter style functions acting directly upon a NonEmptyArray which potentially return an Option<NonEmptyArray>, implicitly convert to an Array, or if its clearer to explicitly convert a NonEmptyArray to a normal Array before running filter style functions. Ideally converting from a NonEmptyArray to Array would incur little to no runtime cost.

That said, a quick glance shows these are the missing functions/instances:

  • Compactable
    • compact
    • separate
  • Foldable
    • foldMap
  • FoldableWithIndex
    • foldMapWithIndex
  • FilterableWithIndex
    • All functions missing
  • Traversable
    • All functions missing
  • Unfoldable
    • All functions missing
  • Witherable
    • All functions missing
  • Combinators
    • Lots of inconsistencies here, while the majority of functions exist in both, some only exist in Array while others only exist within NonEmptyArray
    • foldMap and foldMapWithIndex listed here instead of under Foldable and FoldableWithIndex
  • Destructors
    • All find*, fold* and match* missing from NonEmptyArray
  • Instances
    • getMonoid
    • getOrd
  • Unsafe
    • Contains no unsafe methods
  • Utils
    • Many util functions missing from NonEmptyArray

Also of note, the order of the documentation is different for Array vs NonEmptyArray which also further complicates comparison between the two different Array types.

Your environment

Software Version(s)
fp-ts 2.9.5
TypeScript 4.1
@gcanti
Copy link
Owner

gcanti commented Mar 3, 2021

Thanks @jleider this kind of analysis is gold!

Ideally converting from a NonEmptyArray to Array would incur little to no runtime cost.

Note that you don't need any conversion, a NonEmptyArray is an Array, so all Array APIs can be used with a NonEmptyArray without further ado.

As a general design decision I think that NonEmptyArray should only contain "NonEmpty" specific APIs, i.e.

  1. APIs which return NonEmptyArray in the output type (e.g. prepend, append)
  2. APIs which return Option<NonEmptyArray> in the output type where the failure is not related to emptyness (e.g. insertAt, updateAt)

Point 1. means that, for example, NonEmptyArray.filter and NonEmptyArray.filterWithIndex can be deprecated (in favour of Array's filter and filterWithIndex).

That said, a quick glance shows these are the missing functions/instances:

  • Compactable: we can't define a Compactable instance for NonEmptyArray
  • Foldable: instance already defined (since 2.7.0)
  • FoldableWithIndex: instance already defined (since 2.7.0)
  • FilterableWithIndex: we can't define a FilterableWithIndex instance for NonEmptyArray
  • Traversable: instance already defined (since 2.7.0)
  • Unfoldable: we can't define a Unfoldable instance for NonEmptyArray
  • Witherable: we can't define a Witherable instance for NonEmptyArray
  • Combinators
    • some only exist in Array while others only exist within NonEmptyArray: this is intended, see 1. and 2.
    • foldMap and foldMapWithIndex listed here instead of under Foldable and FoldableWithIndex: that's because technically they are different from the type class operations, the constraint is different (Semigroup vs Monoid)
  • Destructors
    • All find*, fold* and match* missing from NonEmptyArray: that's because they are not specific to NonEmptyArray
  • Instances
    • getMonoid: we can't define a Monoid instance for NonEmptyArray
    • getOrd: not specific to NonEmptyArray
  • Unsafe: not specific to NonEmptyArray (well maybe unsafeUpdateAt). Also I'm thinking about deprecating those unsafe APIs in future relases.
  • Utils: we should scan these APIs one by one to see if they make sense as NonEmptyArray APIs (e.g. uniq)

@gcanti
Copy link
Owner

gcanti commented Mar 3, 2021

Thinking twice matchLeft and matchRight would actually make sense for NonEmptyArray / ReadonlyNonEmptyArray:

export const matchLeft = <A, B>(f: (head: A, tail: Array<A>) => B) => (as: NonEmptyArray<A>): B => f(head(as), tail(as))

export const matchRight = <A, B>(f: (init: Array<A>, last: A) => B) => (as: NonEmptyArray<A>): B =>
  f(init(as), last(as))

@gcanti gcanti added this to the 2.11 milestone Mar 3, 2021
@mlegenhausen
Copy link
Collaborator

matchLeft and matchRight are awesome!

What I have added to my own extensions is a modifyLeft and modifyRight function for NonEmptyArray.

export function modifyLeft<A>(f: (a: A) => A): (nea: NonEmptyArray<A>) => NonEmptyArray<A> {
  return matchLeft((head, tail) => pipe(tail, prepend(f(head))))
}

export function modifyRight<A>(f: (a: A) => A): (nea: NonEmptyArray<A>) => NonEmptyArray<A> {
  return matchRight((init, last) => pipe(init, append(f(last))))
}

Adding uniq to NonEmptyArray would be awesome!

@gcanti
Copy link
Owner

gcanti commented Mar 4, 2021

@mlegenhausen 👍 I would name them modifyHead and modifyLast though.

After scanning the Array / ReadonlyArray modules I propose the following:

2.10.0-rc.4

  • Deprecations
    • NonEmptyArray / ReadonlyNonEmptyArray
      • deprecate filter, filterWithIndex, use Array / ReadonlyArray APIs
  • Polish
    • NonEmptyArray / ReadonlyNonEmptyArray
      • remove duplicated append, prepend, isNonEmpty (added for v2.10, it's not a breaking change), use Array / ReadonlyArray APIs

2.11

  • Deprecations
    • Array / ReadonlyArray
      • deprecate range, use NonEmptyArray / ReadonlyNonEmptyArray APIs
    • NonEmptyArray / ReadonlyNonEmptyArray
      • deprecate concat, use Array / ReadonlyArray APIs
  • New Feature
    • Array / ReadonlyArray
      • add concat, concatW
      • add match, matchW, matchLeftW, matchRightW
    • NonEmptyArray / ReadonlyNonEmptyArray
      • add matchLeft
      • add matchRight
      • add comprehension (without the g filter)
      • add makeBy
      • add modifyHead
      • add modifyLast
      • add updateHead
      • add updateLast
      • add range
      • add replicate
      • add rotate
      • add sortBy
      • add union
      • add uniq

@mlegenhausen
Copy link
Collaborator

If we distinguish more and more between NonEmptyArray and Array method should we not also deprecate Array#of in favor of NonEmptyArray#of? Of cause we can not remove it completely.

@gcanti
Copy link
Owner

gcanti commented Mar 4, 2021

@mlegenhausen I would keep Array's of for completeness' sake, of is exported in all modules as a member of the Pointed type class (I can reuse the NonEmptyArray implementation though).

@jleider
Copy link
Contributor Author

jleider commented Mar 4, 2021

Note that you don't need any conversion, a NonEmptyArray is an Array, so all Array APIs can be used with a NonEmptyArray without further ado.

This is great to know. Maybe you could add that bit of info to the top of the NonEmptyArray docs? At first glance I would not expect to be able to pass a NonEmptyArray<A> to a function expecting an Array<A>.

@gcanti gcanti mentioned this issue Mar 26, 2021
@cdimitroulas
Copy link
Contributor

I'll open a PR to add matchLeft, matchRight, modifyHead and modifyLast to NonEmptyArray & ReadonlyNonEmptyArray

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

4 participants