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

Feat: Conditionally applied methods #99

Closed
SFM61319 opened this issue Feb 21, 2024 · 9 comments · Fixed by #101
Closed

Feat: Conditionally applied methods #99

SFM61319 opened this issue Feb 21, 2024 · 9 comments · Fixed by #101

Comments

@SFM61319
Copy link
Contributor

SFM61319 commented Feb 21, 2024

Coming from rust-itertools/itertools#881 - methods that let you apply other methods conditionally would be helpful.
Something that lets you do:

fn u8_to_bools(value: u8, rtl: bool) -> [bool; 8] {
    let mut result = [false; 8];
    result
        .iter_mut()
        // Applies `Iterator::rev` if and only if `rtl` is `true`.
        // Otherwise does nothing.
        .apply_if(rtl, Iterator::rev)
        .enumerate()
        .for_each(|(i, b)| *b = ((value >> i) & 1) == 1);

    result
}

Along with other methods for ternary conditions, etc., like:

fn get_pairs(bytes: &[u8], overlap: bool) -> Vec<(u8, u8)> {
    bytes
        .iter()
        .copied()
        // Applies `Itertools::tuple_windows` if and only if the `overlap` is `true`.
        // Otherwise applies `Itertools::tuples`.
        .apply_either(overlap, Itertools::tuple_windows, Itertools::tuples)
        .collect()
}

While these examples revolve around iterators, the scope itself of these conditionally applied methods is much larger and in my humble opinion very useful.

It's simple to implement and could be very useful, especially when you have a large method chain and breaking it up for an if[-else] block would negatively affect the readability (see my doc examples).

I have already written a simple implementation on my local fork of either. Correct me if I'm wrong, but this only needs minor changes to work with the either crate instead.

If this feature request gets accepted, I would love to contribute to it myself.

@Philippe-Cholet
Copy link

It would require a new trait in this crate which I would not qualify as "minor change" but I would definitely like to use this myself. Alternatively or additionally to this, I've thought of an EitherExt::into_either which might have a wider usage.

PS: I see this repo has been moved to rayon-rs recently, congrats.

SFM61319 added a commit to SFM61319/rust-either that referenced this issue Feb 21, 2024
Add:

+ Trait `ApplyEither`
  - `apply_either`: Conditionally choose a function and apply it.
  - `apply_if`: Conditionally choose to apply a function.
+ Impl `ApplyEither` for generic type `T`, where `T` is `Sized`

Close Feat: Conditionally chained methods rayon-rs#99
SFM61319 added a commit to SFM61319/rust-either that referenced this issue Feb 21, 2024
Add:

+ Trait `ApplyEither`
  - `apply_either`: Conditionally choose a function and apply it.
  - `apply_if`: Conditionally choose to apply a function.
+ Impl `ApplyEither` for generic type `T`, where `T` is `Sized`

Close Feat: Conditionally chained methods rayon-rs#99
@SFM61319
Copy link
Contributor Author

Wrote a rough implementation for the feature (keeping @Philippe-Cholet's suggestion for an into_either method in mind) in my fork.

Accepting this feature request (followed by me drafting a PR and getting the code reviewed) is the only delay now. 😆

@SFM61319 SFM61319 changed the title Feat: Conditionally chained methods Feat: Conditionally applied methods Feb 21, 2024
@SFM61319
Copy link
Contributor Author

Hi @cuviper (I apologize for the ping. I pinged you because you were the most active contributor), I just wanted to know if this feature request will be accepted (since this issue and my branch are starting to go stale). I have an implementation on my fork, ready to be reviewed and merged.

@cuviper
Copy link
Member

cuviper commented Feb 28, 2024

I think I would prefer just the into_either method for now. I doubt the utility of into_left/right when you can just wrap an expression in Either::Left(_) or Right(_), and the "apply" methods are just maps after into_either. And in particular, apply_if is too left-oriented, when Either tries not to place any "preference" on left or right.

@SFM61319
Copy link
Contributor Author

SFM61319 commented Feb 28, 2024

I doubt the utility of into_left/right when you can just wrap an expression in Either::Left(_) or Right(_)

I thought having simple functions like into_left/right would make things a bit easier and cleaner (similar to is_left/right methods which aren't "necessary" because of if-let and match expressions). Like:

some_foo
    .into_left()
    .some_bar()
    .some_baz()

over

Either::Left(some_foo)
    .some_bar()
    .some_baz()

And also lets you use it anywhere in the middle of a method chain. Like:

some_foo
    .some_bar()
    .into_left()
    .some_baz()

Just my two cents.

The "apply" methods are just maps after into_either. As in particular, apply_if is too left-oriented, when Either tries not to place any "preference" on left or right.

Well in that case I will get started on implementing an apply_if_not method then! Just kidding (unless you guys like the idea of course).

In all seriousness, depending on the final decision, I will probably make a tiny crate that provides this functionality using either then.

@cuviper
Copy link
Member

cuviper commented Mar 2, 2024

Another issue I have with into_left/right is that I think it's too restrictive to require the same type on both sides. That is, when wrapping Left, I think it's more common that you'll have a different type on the Right, and vice versa.

A closure version of into_either, might be nice, something like:

    fn into_either_with(self, f: impl FnOnce(&Self) -> bool) -> Either<Self, Self>;

I will probably make a tiny crate that provides this functionality using either then.

Sure, you're free to do so, of course. That's especially useful when there are potential design choices that aren't clear yet, so you'll be able to make breaking changes independently of either v1 semver.

@Philippe-Cholet
Copy link

@SFM61319 Do you make a PR for IntoEither::into_either[_with]?

@SFM61319
Copy link
Contributor Author

@Philippe-Cholet I will make one now.

SFM61319 added a commit to SFM61319/rust-either that referenced this issue Mar 13, 2024
Add:

+ Trait `IntoEither`
  - `into_either`: Conditionally convert any sized type into any variant of `Either`.
  - `into_either_with`: Like `into_either`, but takes a predicate function instead.
+ Impl `IntoEither` for generic type `T`, where `T` is `Sized`

Close rayon-rs#99
@SFM61319
Copy link
Contributor Author

@Philippe-Cholet by the way, I made a crate named apply_conditionally for conditional chaining.

SFM61319 added a commit to SFM61319/rust-either that referenced this issue Apr 11, 2024
Add:

+ Trait `IntoEither`
  - `into_either`: Conditionally convert any sized type into any variant of `Either`.
  - `into_either_with`: Like `into_either`, but takes a predicate function instead.
+ Impl `IntoEither` for generic type `T`, where `T` is `Sized`

Close rayon-rs#99
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 a pull request may close this issue.

3 participants