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 is_first/is_last helpers to Position #1019

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

Conversation

xmo-odoo
Copy link

They're just convenience method, but avoid having to dereference individual variants, and may mitigate missing the Only case.

They're just convenience method, but avoid having to dereference individual variants, and may mitigate missing the `Only` case.
@xmo-odoo
Copy link
Author

Originally proposed by bluss (#169 (comment))

Do make the first/last cases clearer and somewhat less error prone e.g.

use itertools::Itertools;

let sentence = "The quick brown fox.";
let words = sentence.split(' ');

for (pos, word) in words.with_position() {
    if let itertools::Position::Last | itertools::Position::Only = pos {
        print!("{}", word);
    }
}

becomes

use itertools::Itertools;

let sentence = "The quick brown fox.";
let words = sentence.split(' ');

for (pos, word) in words.with_position() {
    if pos.is_last() {
        print!("{}", word);
    }
}

@phimuemue
Copy link
Member

Hi there, I'm unsure that an is_first that checks for First || Only is a good idea (I naively assumed a shorthand for only First). If we accept this, I suggest renaming to is_first_or_only so that the documentation guides the reader to the fact a single element is first, too.

But then, is_first_or_only only sugar coats the match, and I usually advocate against such thin wrappers.

Thus, I'm reluctant to merge this. @jswrenn Opinions?

@xmo-odoo
Copy link
Author

Hi there, I'm unsure that an is_first that checks for First || Only is a good idea (I naively assumed a shorthand for only First).

To me that is the entire point, it's a predicate telling you whether an element is the first / last of the sequence regardless of the sequence's dimensions, so you don't need to know / care about the Only case.

I was reminder of this inconvenience upon stumbling (back) on https://stackoverflow.com/a/76223155/8182118

@jswrenn
Copy link
Member

jswrenn commented Feb 12, 2025

In this case, I think the chainability of the method tips the scales towards the shorthand being compelling. (As opposed to .map(...) shorthands, since the long-form is already chainable.) I agree with @phimuemue that is_first_or_only is worth the few extra keystrokes for the extra clarity.

@phimuemue
Copy link
Member

To me that is the entire point, it's a predicate telling you whether an element is the first / last of the sequence regardless of the sequence's dimensions, so you don't need to know / care about the Only case.

I was reminder of this inconvenience upon stumbling (back) on https://stackoverflow.com/a/76223155/8182118

I appreciate the reasoning, but it goes against Rust's convention:

  • enum Result{Ok, Err} has is_ok, is_err
  • enum Option{Some, None} has is_some, is_none
  • enum ControlFlow{Continue, Break} has is_continue, is_break
  • enum Poll{Ready, Pending} has is_ready, is_pending
  • enum Cow{Borrowed, Owned} has (experimental) is_borrowed, is_owned

I.e. is_-functions refer to one specific variant of the enum. Thus, I'd expect is_first to match First - but since this is probably not intentional I suggest renaming it to is_first_or_only so that the programmer gets a compiler error when trying is_first and is pushed towards thinking about the Only case.

(Notably, enum Ordering{Less, Equal, Greater} does not have is_less et al, but has has is_lt and is_le.)

@phimuemue
Copy link
Member

In this case, I think the chainability of the method tips the scales towards the shorthand being compelling. (As opposed to .map(...) shorthands, since the long-form is already chainable.) I agree with @phimuemue that is_first_or_only is worth the few extra keystrokes for the extra clarity.

@jswrenn What does "chainability" exactly mean? Is it that e.g. for an Option<Position>, we can write option.map(Position::is_first_or_only) instead of option.map(|position| matches!(position, First|Only))?

@jswrenn
Copy link
Member

jswrenn commented Feb 17, 2025

I mean that iter.foo().bar().map_unwrap().baz() isn't much of a win over iter.foo().bar().map(Option::unwrap).baz(), because it only saves a few keystrokes, but iter.foo().bar().baz() is a bigger win over bar(iter.foo()).baz(), because it preserves a left-to-right reading order (i.e., chainability).

I'm more amenable to shorthands when they preserve chainability than when they merely safe keystrokes.

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