is_first/is_last to ease handling of Position::Only

[kornelski]

I wanted to handle the last (final) element of an iterator differently, and used if position == Position::Last {…}, which turned out to be a bug-causing gotcha, because Position::Only is also an element after which there are no more elements left.

Having to write position == Position::Last || position == Position::Only makes this a bit verbose.

I've changed Position to be a struct to make it easier to check the conditions separately. This is a semver-major change.

[kornelski]

Theoretically, it would also be possible to implement PartialEq so that Position::Only == Position::Last, but that would be a breaking change, and could be even more surprising.

[phimuemue]

Good Observation, but I am pretty Sure that is_last does not fully solve The Problem because people still use if let Last(x) = ..., and we should, If we Touch this, try to fix The root of The Problem. I think renaming The enum variants to ExactlyOne, FirstOfMany, ... would solved it. Surely it ist a bit cumbersome, but imho we should prefer correctness over "ergonomics".

Related: #1019

@jswrenn thoughts?

[kornelski]

Renaming the enum variants may help draw the attention to the footgun, but it doesn't improve its poor ergonomics (still requiring two comparisons with even more verbose variant names). itertools doesn't do anything that isn't possible otherwise with more clunky code, so having a clunky API in itertools kinda defeats its purpose.

I would prefer not to make an incompatible change, because IMHO itertools makes too many semver-major releases, and it's hard to keep up and avoid duplicates of itertools in my dependency tree.

But if you're going to change it in an incompatible way, just stop using enum. It's a wrong tool for the problem. It's a sum type trying to represent a product type.

pub struct Position {
    pub first: bool,
    pub last: bool,
}

and you can add is_* methods without worrying about breaking conventions of sum types.

[jswrenn]

Having to write position == Position::Last || position == Position::Only makes this a bit verbose.

You might consider instead writing matches!(position, Last | Only).

I think renaming The enum variants to ExactlyOne, FirstOfMany, ... would solved it.

@phimuemue I agree, and I think it'd complement quite well with is_first and is_last predicates.

IMHO itertools makes too many semver-major releases

A core part of this crate's mission is to serve as a laboratory for iterator adaptors before they're uplifted into the standard library. We're keen to fiddle with ergonomics and willing to make breaking changes because once an adaptor is uplifted to std, that luxury vanishes.

In this case, we can ease the pain the of break by re-exporting deprecated aliases for these variants.

[scottmcm]

does not fully solve The Problem because people still use if let Last(x) = ..., and we should, If we Touch this, try to fix The root of The Problem.

Hmm, I wonder if expression the 4 variants as 2x2 could be a way to address this?

Like suppose instead of returning Item = Position<T>, it returned (IsFirst, T, IsLast) or something.

Then you get (IsFirst::Yes, _, IsLast::Yes) instead of Only, so looking for just IsFirst works for Only too. Then for multiple elements you get (IsFirst::Yes, _, IsLast::No), (IsFirst::No, _, IsLast::No), …, (IsFirst::No, _, IsLast::No), (IsFirst::No, _, IsLast::Yes).

(Hopefully someone else can come up with a better naming for those enums, though. I think different types are better than just bools, though, to reduce the chance of looking at the wrong one. But I guess named fields could also work, like if let WithFirstLast { first: true, value, .. } = …, iterating over Item = WithFirstLast<T>.)

[jswrenn]

Riffing off @scottmcm's proposal, perhaps we should define Position like this:

#[derive(Copy, Clone)]
struct Position {
    pub is_last: bool,
    pub is_first: bool,
}

impl Position {
    pub fn is_middle(&self) -> bool {
        !self.is_first && !self.is_last
    }

    pub fn is_only(&self) -> bool {
        self.is_first && self.is_last
    }
}

This would be amenable to pattern matching and still allow us to define helpful predicates on Position.

[kornelski]

My usecase looks like this (using all elements, but treating the last specially)

for (pos, widget) in widgets {
   …
   if !pos.is_last() { separator(); }
}

(in that case the code is imperative, and the items don't have the same type, so intersperse() doesn't help)

Destructuring from a wrapper struct would not be useful for me. It makes it harder to name the variable:

for WrapperStruct { is_last, value: widget, .. } in widgets {

(and I keep forgetting is it widget: value or value: widget?)

and adding some if let/let else/match to check a boolean is overcomplicating things here. If there was a wrapper, I'd rather do:

for wrapped in widgets {
    let widget = wrapped.value;
    if !wrapped.is_last { … }
}

but that's still worse than a tuple.

for (_, widget, is_last) in widgets {
   …
   if is_last == IsLast::No { separator(); }
}

This is tolerable, but having a Yes/No enum seems silly, and it's all more complicated than pos.is_last() or pos.is_last I'm proposing.

[jswrenn]

In my proposal, your use-case would be written as:

for (pos, widget) in widgets {
   …
   if !pos.is_last { separator(); }
}

There's no need to destructure.

[phimuemue]

Riffing off @scottmcm's proposal, perhaps we should define Position like this:

#[derive(Copy, Clone)]
struct Position {
    pub is_last: bool,
    pub is_first: bool,
}

impl Position {
    pub fn is_middle(&self) -> bool {
        !self.is_first && !self.is_last
    }

    pub fn is_only(&self) -> bool {
        self.is_first && self.is_last
    }
}

This would be amenable to pattern matching and still allow us to define helpful predicates on Position.

Let's do this. It (hopefully) draws the attention to the fact that is_first and is_last are not mutually exclusive.

Shold we make is_first/last public or keep them private and offer functions is_first/last()?

I'm still thinking if we should rename to is_exactly_one, because we have exactly_one as a function, and I'd like to keep them in line. (Alternatively, rename exactly_one to only.)

[phimuemue]

(in that case the code is imperative, and the items don't have the same type, so intersperse() doesn't help)

@kornelski Just for completeness, because I had similar situations: I almost always end up with one of intersperse[_with], interleave[_shortest], merge[[_join]_by], or - if efficiency is needed - maybe with manually splitting off the first element, so the others can be prepended with separator unconditionally.

[codecov]

Codecov Report

Attention: Patch coverage is 76.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 94.12%. Comparing base (6814180) to head (b319a9b).
Report is 154 commits behind head on master.

Files with missing lines	Patch %	Lines
src/with_position.rs	68.42%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1042      +/-   ##
==========================================
- Coverage   94.38%   94.12%   -0.27%     
==========================================
  Files          48       50       +2     
  Lines        6665     6249     -416     
==========================================
- Hits         6291     5882     -409     
+ Misses        374      367       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kornelski]

I've updated the code to what's been discussed here.

CI needs #1044

[phimuemue]

Thank you.

@jswrenn I'd be fine with that. So if you don't object, we can merge this.

[phimuemue]

Thank you.