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

Adjacent pairs wrapping #141

Closed
wants to merge 7 commits into from
Closed

Conversation

mdznr
Copy link
Contributor

@mdznr mdznr commented May 7, 2021

Description

Adds an optional wrapping parameter to adjacentPairs algorithm that allows you to include a pair of the last and first element.

Detailed Design

extension Sequence {
  /// …
  ///
  /// The following example uses `adjacentPairs(wrapping: true)` to iterate over
  /// adjacent pairs of integers, including the last and first:
  ///
  ///     for pair in (1...).prefix(5).adjacentPairs(wrapping: true) {
  ///         print(pair)
  ///     }
  ///     // Prints "(1, 2)"
  ///     // Prints "(2, 3)"
  ///     // Prints "(3, 4)"
  ///     // Prints "(4, 5)"
  ///     // Prints "(5, 1)"
  ///
  /// - Parameter wrapping: If `true`, include the pair of the last and first
  /// elements of the sequence as the last elements of the returned collection.
  /// Defaults to `false`.
  @inlinable
  public func adjacentPairs(wrapping: Bool = false) -> AdjacentPairsSequence<Self> {
    AdjacentPairsSequence(base: self, wrapping: wrapping)
  }
}

extension Collection {
  /// …
  ///
  /// - Parameter wrapping: If `true`, include the pair of the last and first
  /// elements of the collection as the last elements of the returned
  /// collection. Defaults to `false`.
  @inlinable
  public func adjacentPairs(wrapping: Bool = false) -> AdjacentPairsCollection<Self> {
    AdjacentPairsCollection(base: self, wrapping: wrapping)
  }
}

Alternatives Considered

  • A new Sequence and Collection algorithm. This would have resulted in a lot of duplicate code from AdjacentPairs that would be better shared between the implementations.

Documentation Plan

I’ll waiting to hear feedback on whether or not this is desirable and on the direction before diving into the documentation too much.

  • Improve inline documentation
  • Update Guides/AdjacentPairs.md
  • README.md

Test Plan

For each of the existing adjacentPairs unit tests, I’ve added one that tests the wrapping behavior as well.

Source Impact

Existing calls to adjacentPairs() continues to work as-is since the wrapping parameter has a default value of false.

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

@mdznr mdznr linked an issue May 7, 2021 that may be closed by this pull request
@mdznr mdznr force-pushed the adjacent_pairs_wrapping branch from 98a5278 to f578c98 Compare May 7, 2021 04:03
@mdznr mdznr force-pushed the adjacent_pairs_wrapping branch from f578c98 to 1370d96 Compare May 7, 2021 04:04
@xwu
Copy link
Contributor

xwu commented May 9, 2021

wrapping suggests to me an infinite sequence (which I'd imagine has its own uses); is there potentially another way of describing this specific option?

@mdznr
Copy link
Contributor Author

mdznr commented May 10, 2021

wrapping suggests to me an infinite sequence (which I'd imagine has its own uses); is there potentially another way of describing this specific option?

This is something that I struggled with. I was hoping to get some feedback on the naming. I agree that wrapping could be misinterpreted, but haven’t thought of anything better yet.

Here are some other options to get the discussion going:

  • includingEnds
  • includingAdjoinedEnds
  • includingLastAndFirstPair
  • adjoiningLastAndFirst
  • adjoiningEnds
  • Something using clasp (as used with jewelry to join together a series of links into a loop)

@kylemacomber
Copy link
Contributor

If we added this, I presume we would want to add the same functionality to windows(ofCount:) and there'd be count - 1 wrapping entries:

let x = [1, 2, 3, 4, 5]
for window in x.windows(ofCount: 3, wrapping: true) {
  print(window)
}
// Prints [1, 2, 3]
// Prints [2, 3, 4]
// Prints [3, 4, 5]
// Prints [4, 5, 1]
// Prints [5, 1, 2]

@mdznr
Copy link
Contributor Author

mdznr commented May 11, 2021

If we added this, I presume we would want to add the same functionality to windows(ofCount:) and there'd be count - 1 wrapping entries

I think you meant count (one array element in the original collection).

Due to the similarities between windows(ofCount: 2) and adjacentPairs(), it would seem we’d want to keep them in sync. However, I don’t see as strong of a use case for wrapping with a count other than 2. Perhaps that’s just because my original use case was only to help link up a circular linked list.

Perhaps instead of adding a wrapped parameter to the adjacentPairs() function, we could create a different function for the purposes of the public API (even if it shared the implementation with AdjacentPairsSequence and AdjacentPairsCollection).

@kylemacomber
Copy link
Contributor

I appreciate that this was motivated by a concrete real world use case, however after thinking about this more, I'm not sure adding a wrapping flag (regardless of its name) pulls its weight.

For example, I think this behavior can be relatively trivially composed out of chain(x, x.prefix(1)).adjacentPairs():

let x = (1...).prefix(5)
for pair in chain(x, x.prefix(1)).adjacentPairs() {
    print(pair)
}
// Prints "(1, 2)"
// Prints "(2, 3)"
// Prints "(3, 4)"
// Prints "(4, 5)"
// Prints "(5, 1)"

I vote that we close this for now and revisit if additional concrete use cases arise.

@natecook1000
Copy link
Member

Agreed, we can close this one for now. Thanks for testing this out, @mdznr — happy to discuss this more on the forums!

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.

AdjacentPairs: Ability to also include the (last, first) pair
4 participants