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

Adds collect methods for arrays of results and maybes #46

Merged
merged 16 commits into from
Jun 5, 2024

Conversation

JackSpagnoli
Copy link
Contributor

@JackSpagnoli JackSpagnoli commented Jun 3, 2024

Adds collect methods for arrays of results and maybes

Adds function collect as Array<Result<O,E>> -> Result<Array<O>, Array<E>> and Array<Maybe<O>> -> Maybe<Array<O>>.

For results, if any result in array is Err, returns an array of all Errs, else returns an array of all Oks.
For maybes, if any result in array is Nothing, returns Nothing, else returns an array of all Justs.

Updates to version 2.3.0.

  • I have added or updated unit tests for this change.
  • I have updated the changelog with info about the changes in this PR

@JackSpagnoli
Copy link
Contributor Author

JackSpagnoli commented Jun 3, 2024

I've not handled the case of empty array's as I'm not sure what you think's best.

Currently [] as Array<Maybe<any>> gets collected to Just([]). I suppose this allows for iterator methods to be chained from it which might be what you'd want?

[] as Array<Result<any, any>> is a bit of a problem. I don't know how we can check that an empty array is of type Result to return Ok or Err, so currently it falls through to returning Just([]) like with a Maybe. The issue is the type signature makes this look like a Result so it's effectively unusable in this case. I could easily break this out into 2 functions, but then they'd have to named differently. What do you think is best?

@rametta
Copy link
Owner

rametta commented Jun 3, 2024

Hello @JackSpagnoli, this is interesting. I like the idea, but I don't think we should be modifying the Array prototype. Would you be able to update and make it a utility like the justs function? Thanks!

Comment on lines +44 to +47
if (failures.length > 0) {
return Err(failures)
}
return Ok(successes)
Copy link
Owner

Choose a reason for hiding this comment

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

Is there an existing collect function that you are modelling this logic off-of? I'm just curious how other libraries are using this

Copy link
Contributor Author

@JackSpagnoli JackSpagnoli Jun 3, 2024

Choose a reason for hiding this comment

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

Not really, I was trying pratica as part of another project and wrote this when verifying and translating an array to allow for code that looked like inputs.map(translate).collect().
I thought it might be a nice inclusion here, but I can't say I've done any research into wider implementations.

Copy link
Contributor Author

@JackSpagnoli JackSpagnoli Jun 3, 2024

Choose a reason for hiding this comment

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

I've done some digging, turns out I did something similar in Rust ages ago, which explains why I thought to call it collect. Only difference is Rust returns Err with the first found error, so Vec<Result<T, E>> -> Result<Vec<T>, E>, rather than Vec<Result<T, E>> -> Result<Vec<T>, Vec<E>>

fn main() {
    let bad_values: Vec<Result<u32, &str>> = vec![Ok(1), Err("error"), Ok(3)];
    let bad_collection = bad_values.into_iter().collect::<Result<Vec<_>, _>>();

    assert!(bad_collection.is_err());
    assert_eq!(bad_collection.unwrap_err(), "error");

    let good_values: Vec<Result<u32, &str>> = vec![Ok(1), Ok(2), Ok(3)];
    let good_collection = good_values.into_iter().collect::<Result<Vec<_>, _>>();

    assert!(good_collection.is_ok());
    assert_eq!(good_collection.unwrap(), vec![1, 2, 3]);
}

@JackSpagnoli
Copy link
Contributor Author

Refactored to a utility function f4032cb

src/collect.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Owner

@rametta rametta left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@rametta rametta merged commit bf711b9 into rametta:master Jun 5, 2024
3 checks passed
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.

2 participants