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

Weird function signature on (mo.Option).Map #21

Open
DylanRJohnston-FZ opened this issue Mar 20, 2023 · 3 comments
Open

Weird function signature on (mo.Option).Map #21

DylanRJohnston-FZ opened this issue Mar 20, 2023 · 3 comments

Comments

@DylanRJohnston-FZ
Copy link

DylanRJohnston-FZ commented Mar 20, 2023

Firstly I just want to say that I love this library and has allowed to me to largely remove my own custom implementation of a lot of these types. However I was wondering what the reasoning behind the signature and behaviour of (mo.Option).Map?

func (o Option[T]) Map(mapper func(value T) (T, bool)) Option[T] {
	if o.isPresent {
		return TupleToOption(mapper(o.value))
	}

	return None[T]()
}

Generally speaking Map called on a Some should always return a Some which is a sentiment you also shared here. The ability to return false within the mapper makes Map behave more like FlatMap. It also makes passing point-free functions a little awkward as you have to "lift" the mapper into that signature. e.g.

// Error function signature requires returning an additional boolean
mo.Some("ExampleString").Map(strings.ToLower)

func lift[A any](fn func(A) A) func(A) (A, bool) {
  return func(a A) (A, bool) {
    return fn(a), true
  }
}

mo.Some("ExampleString").Map(lift(strings.ToLower))

I propose that (mo.Option).Map's signature should be changed to remove the boolean argument, if someone wants the old behaviour they can use FlatMap and TupleToOption.

@DylanRJohnston-FZ DylanRJohnston-FZ changed the title Weird function signature on (*mo.Option).Map Weird function signature on (mo.Option).Map Mar 20, 2023
@samber
Copy link
Owner

samber commented Mar 20, 2023

Ok, I see. 👍

I will mark this issue with a "breaking-change" tag, for preparing v2. Please create an issue for any unexpected API. 🙏

Let solves all these little things before a new major release later this year!

@calvinbrown085
Copy link

Just wanted to give a +1 on this, coming from cats and Scala this map implementation had me scratching my head a bit.

@dicky-cat
Copy link

Taeekkkk

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

No branches or pull requests

4 participants