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 product_ok and sum_ok #814

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sjackman
Copy link

@sjackman sjackman commented Dec 7, 2023

  • .try_product() is a more convenient way of writing .product::<Result<_, _>>()
  • .try_sum() is a more convenient way of writing .sum::<Result<_, _>>()

Modelled after .try_collect()

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented Dec 9, 2023

We have a bunch of methods about iterators of results: map_ok filter_ok filter_map_ok flatten_ok fold_ok (and related but different process_results) and try_collect clashing with the nightly Iterator::try_collect.

I think try_ methods should be reserved to (unstable for now) Try trait usages.
For consistency, I would suggest names to be product_ok and sum_ok.

Now, do we want to add those shortcuts? That's the question. @jswrenn What do you think?

EDIT: The documentation of sum (same for product) mention panics. It would do the same here.

Panics

When calling sum() and a primitive integer type is being returned, this method will panic if the computation overflows and debug assertions are enabled.

I failed to read the documentation once but if this is not mentioned then someone will have a problem.

@sjackman
Copy link
Author

sjackman commented Dec 9, 2023

For consistency, I would suggest names to be product_ok and sum_ok.

Done!

@sjackman sjackman changed the title Add try_product and try_sum Add product_ok and sum_ok Dec 9, 2023
`.try_product()` is a more convenient way of writing `.product::<Result<_, _>>()`
`.try_sum()` is a more convenient way of writing `.sum::<Result<_, _>>()`
@sjackman
Copy link
Author

sjackman commented Dec 9, 2023

EDIT: The documentation of sum (same for product) mention panics. It would do the same here.

Done!

@Philippe-Cholet
Copy link
Member

@jswrenn Merry Xmas. What do you think about adding product_ok and sum_ok?
(There is also #830 about a fold_while_ok?)

/// Ok(())
/// }
/// ```
fn product_ok<T, U, E>(self) -> Result<U, E>
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this is an inelegantly large number of generic arguments.

This is a fairly radical alternative, but: What if we just mirrored the stdlib Try trait? We can pub-in-priv our Try-fork so that folks don't end up explicitly depending on it, and then simply (and potentially non-breaking-ly!) switch over to the standard library Try once it's stabilized.

If we do this, our *_ok functions will support more than just Results, and they'll be able to take just one generic argument, rather than three (I think).

Thoughts, @Philippe-Cholet and @phimuemue?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

That sure is radical but interesting, maybe it should have its own issue.
About the names, *_ok now and try_* later? (or maybe_*?!)

After a quick look, ControlFlow requires 1.55+ while we currently have 1.43. Unless we copy it too?
I previously read something about #[track_caller] but don't remember much about its use.

In libcore, fold is usually specialized using try_fold specializations alongside with NeverShortCircuit which surely won't be public forever so I thought that we would copy it at some point.

@Philippe-Cholet Philippe-Cholet added the fallible-iterator Iterator of results/options label Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fallible-iterator Iterator of results/options
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants