search

traverse1984 / oxide.ts

Rust's Option<T> and Result<T, E>, implemented for TypeScript.
MIT License
517 stars 19 forks source link

Adds flatten() to Option and Result #6

Closed rrichardson closed 2 years ago

rrichardson commented 2 years ago

This adds the flatten() method to Result and Option

traverse1984 commented 2 years ago

I appreciate your interest in the project, but I'm afraid I can't merge this. The implementation is incredibly limited and makes changes which do not make sense. I don't think it's useful enough in the current form. In my view, flatten should also be recursive.

Additionally, your implementation changes andThen which is not acceptable. The core library needs to remain unchanged, and it shouldn't be necessary to modify an unrelated function to implement a new feature - especially when you could just duplicate the logic if it works similarly.

I think I see what you were going for with the E1 though, but it doesn't actually do what you're thinking it does. This does not work:

const x: Result<Result<number, string>, number> = Ok(Ok(1));
const y = x.flatten(); // this context error
rrichardson commented 2 years ago

Actually.. the change to andThen() is unintentional. That is a remnant of prior experiments. I would remove that.
I don't think multiple error types is something that should be in scope for andThen and flatten.

I adamantly disagree regarding flatten needing to be recursive. There isn't a single implementation that I can think of that flattens recursively (neither Javascript, nor Rust, nor Haskell.)

rrichardson commented 2 years ago

FWIW, I've reverted the andThen type signature in my branch.

traverse1984 commented 2 years ago

Okay, that makes more sense.

Javascripts built-in Array.flat allows you to specify the recursion depth. I probably should have said 'recursive capable'. However to keep with the Rust implementation it might be better to not include recursion depth as an option.

rrichardson commented 2 years ago

In my rust code, I did end up with one particularly hairy bit (processing a stream inside of a stream where both streams returned Results<>) where I got to call .flatMap({...res.into_iter()}).flatten().flatten() but, I am not sure if flatten(3) would be better. the triple flatten call is a good indicator that something strange is going on and is worth extra looks. :)