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

add common extensions for option #2397

Merged
merged 6 commits into from
May 12, 2021

Conversation

myuwono
Copy link
Collaborator

@myuwono myuwono commented May 9, 2021

In this PR

  • added strong Option<A>.pairLeft(l: L): Option<Pair<L, A>> and Option<A>.pairRight(r: R): Option<Pair<A, R>>
  • added List<Option<A>>.flattenOption(): List<A>
  • added Map<K, Option<V>>.filterOption(): Map<K, V>
  • added Sequence<Option<A>>.filterOption(): Sequence<A>
  • added F<A>.traverseOption(f: (A) -> Option<B>): Option<F<B>> and F<Option<T>>.sequence(): Option<F<T>> for either, option, validated, ior, lists, nonemptylists, sequence, and maps.
  • added some missing tests for Traverse

fixes #2374


inline fun <B, C, D> zip(
b: Option<B>,
c: Option<C>,
map: (A, B, C) -> D
): Option<D> =
zip(b, c, Some.unit, Some.unit, Some.unit, Some.unit, Some.unit, Some.unit, Some.unit) { b, c, d, _, _, _, _, _, _, _ -> map(b, c, d) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like these changes are just new formatting? I don't have a preference but just thought I'd mention it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it's only reformatting. it kept doing that everytime i did a reformat code through intellij. My intellij seems to have picked up everything in editor config but the line length settings..

Copy link
Member

@raulraja raulraja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks great, @myuwono! Thanks so much for taking care of this and making it consistent. I've noticed we added the bifunctor methods like bitraverse, but they have no test associated; perhaps adding a couple more tests for those will help as we are about to turn codecov. Someone else would eventually have to add it.

Thanks again!

@raulraja raulraja requested a review from a team May 9, 2021 09:15
@myuwono
Copy link
Collaborator Author

myuwono commented May 9, 2021

thanks @raulraja yeah indeed it did look like those codes were added without tests. I've taken care of those as well.

Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @myuwono for this contribution 😍!! And congratz on your first contribution to the repo 👏 👏 👏 🎉 🎉 🎉

A couple of nits to implement traverse for Option directly to shave off some unnecessary allocations.

myuwono and others added 3 commits May 11, 2021 15:33
Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
@myuwono
Copy link
Collaborator Author

myuwono commented May 11, 2021

@nomisRev I've used direct implementation for traverseOption as suggested, it's definitely much nicer.

Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks @myuwono for the contribution!! 👏 👏 👏 And congrats & welcome with your first contribution in this repo 😁 🎉 🎉 🎉

@myuwono
Copy link
Collaborator Author

myuwono commented May 11, 2021

Whoops seems like i forgot to format Iterable.kt before the previous commit. @nomisRev That's fixed.

@rachelcarmena
Copy link
Member

Welcome to this repository as well @myuwono and thanks!! 🎉

@rachelcarmena rachelcarmena merged commit 4212e57 into arrow-kt:main May 12, 2021
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.

["Request"] Either.toOption() is gone so code completion sugests A?.toOption()
4 participants