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

Fix typings for unfold #506

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix typings for unfold #506

wants to merge 3 commits into from

Conversation

eddiew
Copy link

@eddiew eddiew commented Nov 24, 2017

Summary

The typings for Most.unfold don't match what the code does or the API docs, so I fixed them.

@briancavalier
Copy link
Member

Hey @eddiew, thanks. Hmmm, yes, the type seems wrong. I like your idea to introduce an UnfoldValue sum type. How about this?

type UnfoldValue<S, V> = { done: false, seed: S, value: V } | { done: true, value?: V }

The first variant could use an intersection with the existing SeedValue type--that'd be fine. It needs to have a done: boolean, so maybe like:

type UnfoldValue<S, V> = { done: false } & SeedValue<S, V> | { done: true, value?: V }

I think using an intersection is a bit harder to read, though. What do you think?

@eddiew
Copy link
Author

eddiew commented Dec 4, 2017

I thought done was allowed to be undefined if not true, so then it wouldn't be necessary to have the done: false in the first variant. In that case we could do:

type UnfoldValue<S, V> = SeedValue<S, V> | { done: true, value?: V }

But if you'd rather keep the done: false then I agree that the intersection is a little bit confusing, and I like the first version you have better.

@MaxSvargal
Copy link

Maybe also add transduce to definitions?

@briancavalier
Copy link
Member

Sorry for taking a while to respond!

I thought done was allowed to be undefined if not true

Hmmm, yes, the current docs do indeed read that way, and so does the example. Ok, let's go with allowing done to be missing or undefined. Thanks for pointing that out.

@briancavalier
Copy link
Member

@MaxSvargal Let's do that in another PR. See also: #512 where it's being discussed.

@@ -1,4 +1,5 @@
declare type SeedValue<S, V> = { seed: S, value: V };
declare type UnfoldValue<S, V> = SeedValue<S,V> | { done: boolean }
Copy link
Member

Choose a reason for hiding this comment

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

Since the docs are kind of loose, we may need to be even more permissive. Maybe we can go with this, and then relax it if it causes someone some pain.

Seems like we should tighten the done case to { done: true } since it'd be pointless to return { done: false }. What do you think?

@eddiew
Copy link
Author

eddiew commented Dec 23, 2017

Good catch. I updated the code to require done: true

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.

3 participants