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

All permutations #56

Merged
merged 12 commits into from
Feb 12, 2021
Merged

All permutations #56

merged 12 commits into from
Feb 12, 2021

Conversation

mdznr
Copy link
Contributor

@mdznr mdznr commented Jan 11, 2021

Description

This addition adds the ability to easily get permutations of a range of sizes, instead of the previous behavior of a single, fixed size, k, like #51 did for Combinations.

Detailed Design

The permutations(ofCount:) function that takes a RangeExpression is added in an extension of Collection.

extension Collection {
  public func permutations<R: RangeExpression>(
    ofCount kRange: R
  ) -> Permutations<Self> where R.Bound == Int {
    return Permutations(self, kRange: kRange)
  }
}

If the upper bound of the range exceeds the length of the collection, no such permutations of those sizes will be returned, effectively clamping the range to 0...n where n is the length of the collection.

Naming

The name permutations(ofCount:) is the same as the original, but reflects the ability to provide multiple counts. This is the same pattern used for Combinations.

Documentation Plan

The documentation in Guides/Permutations.md has been updated to reflect the new function. Inline code documentation also exists for each of the functions.

Test Plan

  • Adds tests for the outputs of a range of sizes
  • Adds tests for ranges extending above the size of the collection

Source Impact

The functionality is purely additive. The behavior of the existing function is unchanged.

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 force-pushed the all_permutations branch 2 times, most recently from 4b5269d to 5dcea2c Compare January 16, 2021 19:09
@kylemacomber
Copy link
Contributor

@mdznr Oh I see this is still a draft, let us know when you're ready for us to review it!

@mdznr
Copy link
Contributor Author

mdznr commented Jan 29, 2021

@mdznr Oh I see this is still a draft, let us know when you're ready for us to review it!

I unfortunately haven’t had a lot of time to work on this. The change is very nearly done. It works. The only things left for me was to ensure it was as well-written as it could be, the implementation consistent with Combinations as much as possible, and the set of unit tests are thorough. I was hoping to really dive into those and ensure it was up to standards before asking others to review.

Copy link
Contributor

@timvermeulen timvermeulen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your contribution, @mdznr! This looks very solid, I just have one remark. Feel free to make any other changes you still want to make, but I'm down to get this merged in its current state (modulo that remark).

Sources/Algorithms/Permutations.swift Outdated Show resolved Hide resolved
@mdznr mdznr marked this pull request as ready for review February 10, 2021 00:10
@kylemacomber kylemacomber merged commit d8a11e0 into apple:main Feb 12, 2021
@mdznr mdznr deleted the all_permutations branch February 12, 2021 16:05
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