Add interspersed(with:)

[danielctull]

Description

This addition allows the caller to insert values between each element of a sequence. In this way you could have a string and insert a hyphen between each character or you could have an array of SwiftUI views and insert a separator view between each.

Detailed Design

To achieve this, a new sequence type called Intersperse is added to the library. It also conforms to Collection and BidirectionalCollection when the base sequence conforms to those respective protocols.

For ease of use, a new method is added to sequence to allow the interspersing of the separator element:

extension Sequence {
    func interspersed(with separator: Element) -> Intersperse<Self>
}

It is crucial that the separator is only inserted between elements, so before returning the separator element, a lookup is performed on the base sequence to make sure the next element exists.

Documentation Plan

As the interspersed(with:) method is the main entry point, this is documented with two examples of use which are taken from the included unit tests. The Intersperse sequence type itself is lightly documented, which I believe follows the trend of the current APIs in the library.

Test Plan

Arrays of numbers and a String are used, calling interspersed(with:) on each, a value between the elements of each. I have also tested that the empty sequence case yields an empty sequence.

Source Impact

This is a purely additive change.

Checklist

• I've added at least one test that validates that my change is working, if appropriate
• I've followed the code style of the rest of the project
• I've read the Contribution Guidelines
• I've updated the documentation if necessary

[timvermeulen]

This looks correct, but if you conform Kind to Comparable (which will just correctly use the case order I think?) then this becomes as simple as (lhs.index, lhs.kind) < (rhs.index, rhs.kind)

[timvermeulen]

When traversing the Intersperse indices, each index from the base collection is computed twice: first when the kind of i is .element, and then when the kind is .separator. Maybe we should have the .separator case store the next index, similar to how you solved this problem in Iterator.State?

[danielctull]

Although your explanation is totally coherent, I'm not sure I follow completely. Is the computation you're describing base.index(after: i.index)? Given that i is Intersperse.Index, its index property it holds is not recomputed each time, so should be fine?

In my head, I can vaguely see what you're suggesting. Perhaps I just need to sit with a piece of paper for a few minutes!

[danielctull]

These last couple of commits break Intersperse.Index's conformance to Comparable, I will take a look at this and push a fix for them. 🙂

[CTMacUser]

I think the index comparison can be like:

public struct Index: Comparable {
  enum Representation: Equatable {
    case element(Base.Index)
    case separator(next: Base.Index)
  }
  let representation: Representation

  public static func < (lhs: Index, rhs: Index) -> Bool {
    switch (lhs.representation, rhs.representation) {
    case let (.element(lr), .element(rr)),
         let (.separator(lr), .separator(rr)),
         let (.element(lr), .separator(rr)):
      return lr < rr
    case let (.separator(lr), .element(rr)):
      return lr <= rr
    }
  }
}

The inner index usually cascades, except when comparing an element and separator at the same inner index. Then you need to remember that the separator version is ordered less.

[CTMacUser]

I'm guesstimating, but you probably can make the Collection version conform to RandomAccessCollection when the source does. It's a bunch of calculations, but they don't scale, making the work O(1), suitable for random access.

[timvermeulen]

Here's another possibility, letting tuple comparison + synthesized Comparable do the work:

public struct Index: Comparable {
  enum Kind: Comparable {
    case separator
    case element
  }

  let base: Base.Index
  let kind: Kind

  public static func < (lhs: Index, rhs: Index) -> Bool {
    (lhs.base, lhs.kind) < (rhs.base, rhs.kind)
  }
}

Putting the separator case first makes it correctly compare less than element when the base index is the same.

[danielctull]

I added a test utility to check the order of a Collection's indices were ordered, and used this to test the order of Intersperse's indices which you can see fail on that commit and pass with the commit after with the fix for comparable. I decided to take @CTMacUser's logic, because I feel like it's better to explicitly show that the index stored in separator is actually for the next element, where the index for the element is for the element itself. I appreciate the help with this though @timvermeulen!

[danielctull]

@CTMacUser: I've added the conformance to RandomAccessCollection, which I believe required a custom implementation of distance(from:to) which I also added. I would be interested to hear if I've missed any other requirements for RandomAccessCollection here though. 🙂

This was tricky, so I also added another assert function to make sure the distances are calculated correctly and I've put this in the TestUtilities for tests on other collections to make use of.

[timvermeulen]

RandomAccessCollection also requires efficient implementations of index(_:offsetBy:) and index(_:offsetBy:limitedBy:). It's also fine to leave that as a TODO if that's more convenient, so no problem if you can't get around to implementing those right away.

This was tricky, so I also added another assert function

Both XCTAssertOrderedIndices and XCTAssertIndexDistances are covered by #39 which takes a nearly identical brute-force approach, let's figure out how to go about this 🙂

[danielctull]

I'll add them as TODOs, cheers.

Just taking a gander at your test function and indeed it looks to do the same thing as I was trying to achieve with those two asserts and a little more which is great! One thing I found in writing mine was that it was useful to take into account the endIndex when doing distance checks, because the endIndex is a little weird for Intersperse.

[timvermeulen]

It does take into account endIndex as well, not just the indices that you can subscript the collection with 👍 endIndex is indeed often the most error-prone.

[danielctull]

Using validateIndexTraversals from #39, I found a couple of issues with my implementation of distance(from:to:) when the from index was the endIndex. (Thanks @timvermeulen!)

I also implemented index(_:offsetBy:) which again was made easier to verify using validateIndexTraversals.

[natecook1000]

This returns an invalid index for c.index(after: c.endIndex) — can you add a precondition for that case?

[danielctull]

Good catch, index(before:) also had the same issue. Implemented in 6f6768b.

[natecook1000]

Could you add a test that validates against empty and non-empty sequences? (0...).prefix(_) is an easy way to create something that will call through to sequence-based iteration.

[danielctull]

Ah that makes sense, so that it doesn't go through any of the Collection code? If I've understood correctly, this is in d4bdbc4. 🙂

[ollieatkinson]

It's worth considering what cross-over this implementation has with slidingWindows:

let a = [1, 2, 3, 4, 5]
a.slidingWindows(ofCount: 1).joined(separator: [0]) // 1, 0, 2, 0, 3, 0, 4, 0, 5

extension Collection {
  public func interspersed(with separator: Element) -> JoinedSequence<SlidingWindows<Self>> {
    return slidingWindows(ofCount: 1).joined(separator: [separator])
  }
}

[danielctull]

@ollieatkinson: That's true yeah! One niggle is this is that interspersed(with:) is currently able to work on Sequences, whereas slidingWindows(ofCount:) is only defined on Collection. I also wonder about the overhead of creating intermediate subsequences and concatenating them.

[ollieatkinson]

You are correct SlidingWindows is currently only for Collection, however with enough motivation one would be able to change that.

Both SlidingWindows and JoinedSequence are O(1) for RandomAccessCollection or O(k) (in the example where the window size is 1, k = 1) and O(1) if not, access to the next element is always O(1)

[natecook1000]

@ollieatkinson You don't need the sliding windows adaptor to do this with composition:

let a = [1, 2, 3, 4, 5]
a.lazy.map { CollectionOfOne($0) }.joined(separator: CollectionOfOne(0))

Neither of those would allow you to implement RandomAccessCollection conformance, however, so there's enough benefit to having this as its own adaptor to include it in the package.

[danielctull]

Given this, what is the next step to getting this merged in? Should I squash the commits down and rebase them back onto the main branch?

[natecook1000]

@danielctull All set here, thanks!