Add new `FiniteCycle` type as the return type of `Collection.cycled(times:)` #106

LemonSpike · 2021-03-14T13:24:36Z

This PR aims to fix #99. I have also split up some relevant tests into separate methods for empty collections and changed some tests for FiniteCycle. I have also added tuple labels to Product2 when using the Iterator.next() method and also in the subscript method. This is a cosmetic improvement, but I felt it may read better.

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

… tests.

xwu

👍

xwu · 2021-03-14T14:00:44Z

Sources/Algorithms/Cycle.swift

@@ -56,6 +56,47 @@ extension Cycle: Sequence {

 extension Cycle: LazySequenceProtocol where Base: LazySequenceProtocol {}

+
+/// A collection wrapper that repeats the elements of a base collection for a finite number of times.


Nit: Please wrap to 80 columns, here and elsewhere.

xwu · 2021-03-14T14:02:45Z

Sources/Algorithms/Cycle.swift

+/// A collection wrapper that repeats the elements of a base collection for a finite number of times.
+public struct FiniteCycle<Base: Collection> {
+  /// The collection to repeat.
+  public let base: Base


Let's make this internal, as well as times.

This is because equality (we could conform this type to Equatable, but it doesn't have to be here in this PR) should be based on elementwise comparisons, and [1, 2, 3].cycled(times: 6) should be equivalent to [1, 2, 3, 1, 2, 3].cycled(times: 3). Having base and times publicly accessible would raise the question of whether these properties are "salient" ones for the purposes of considering equivalence.

timvermeulen · 2021-03-18T17:17:01Z

Thanks for taking this on @LemonSpike, this is a great start! Your FiniteCycle correctly delegates all of the work to an underlying Product2<Range<Int>, Base>. However, the real benefit will come from being able to efficiently implement all of the Collection methods that Product2 already implements. By forwarding all public Collection and BidirectionalCollection methods to an underlying Product2 collection, FiniteCycle will have all the same performance characteristics as Product2. Want to have a go at that?

…formance to FiniteCycle.

LemonSpike · 2021-03-20T17:48:51Z

Thanks for the feedback @timvermeulen! I have tried to add the Collection protocol conformances this time by forwarding to an underlying Product2 collection. Are these what you had in mind? Thanks again. 😊

timvermeulen · 2021-03-22T21:29:06Z

This is indeed what I had in mind, great work! The Collection conformance looks good, it implements all the relevant methods. There are just a few things left to do:

We can get rid of the Sequence conformance entirely — normally it's needed in case the base type is only a Sequence and Collection cannot be implemented, but in the case of FiniteCycle we already require that Base: Collection, so it's no longer needed. In the same vein, the type needs LazyCollectionProtocol conformance rather than LazySequenceProtocol.
FiniteCycle is still missing some conformances that Product2 does have, implemented here.
Most public methods are missing @inlinable annotations.
The Cycle.md guide needs a slight tweak to reflect that cycled(times:) now has a new return type.

…tocol conformance.

LemonSpike · 2021-03-24T11:29:10Z

Hi @timvermeulen, thanks for the feedback. Re: your points, I tried to implement your suggestions as follows:

I removed the Sequence conformance entirely. To conform to LazyCollectionProtocol I had to conform to LazySequenceProtocol and LazyCollectionProtocol together where Base: LazyCollectionProtocol, because I couldn't inherit LazySequenceProtocol conformance automatically.
Managed to add these as suggested, but because the FiniteCycle.Index is a typealias for Product2.Index, the Index Hashable conformance was already synthesized.
Added @inlinable to all public methods, by making the let product public.
Markdown file edited but reworded without despite its name since the name no longer includes Sequence.
I changed some @usableFromInline internal methods to @inlinable because of Inline all the things! 🎨 #109.

Let me know your thoughts, thanks.

timvermeulen · 2021-03-31T08:09:27Z

Apologies for the delay, @LemonSpike!

I removed the Sequence conformance entirely. To conform to LazyCollectionProtocol I had to conform to LazySequenceProtocol and LazyCollectionProtocol together where Base: LazyCollectionProtocol, because I couldn't inherit LazySequenceProtocol conformance automatically.

We can get rid of the Iterator type and makeIterator method entirely. The typechecker did get confused about what the Element type is after I tried doing this, so you might need to add typealias Element = Base.Element to the Collection conformance.

Managed to add these as suggested, but because the FiniteCycle.Index is a typealias for Product2.Index, the Index Hashable conformance was already synthesized.

Great point! Let's for now give FiniteCycle its own Index type that wraps a Product2.Index, just so in the future we have the freedom to change the internals if the need ever arises.

Added @inlinable to all public methods, by making the let product public.

While we make all public and internal methods @inlinable, for stored properties @usableFromInline suffices, so there's no need to make the property public.

Markdown file edited but reworded without despite its name since the name no longer includes Sequence.

That looks good, could you also have it mention the conditional LazyCollectionProtocol conformance, similar to the Cycle description? And there's a note a bit higher up about cycled(times:) being a combination of two existing standard library functions which we can probably get rid of altogether.

I changed some @usableFromInline internal methods to @inlinable because of Inline all the things! 🎨 #109.

💯

timvermeulen · 2021-03-31T08:13:05Z

Sources/Algorithms/Product.swift

+    public mutating func next() -> (element1: Base1.Element,
+                                    element2: Base2.Element)? {


Let's keep this tuple unlabeled — Zip2Sequence's element tuple doesn't have labels either, and generic labels such as element1 and element2 don't really add any clarity over .0 and .1.

I am happy to do this, but I still think that the labels improve readability for users who don't know about tuple indexing. I saw we use more descriptive names for public APIs and type names, so I think it could help users understand the API. Let me know your thoughts.

These labels don't improve the users' understanding, because they don't convey any useful information. It's impossible for us to give the tuple elements descriptive names because at this level of abstraction we have nothing to go on. To quote the API Design Guidelines:

Omit needless words. Every word in a name should convey salient information at the use site.

Feel free to start a discussion about this topic on the forums, as this is equally relevant to the Zip2Sequence element type. For the time being we'd like to stick with the precedent set by the standard library.

natecook1000 · 2021-04-07T03:10:07Z

@swift-ci Please test

LemonSpike · 2021-04-07T10:49:26Z

@swift-ci Please test

natecook1000 · 2021-04-07T15:33:00Z

@swift-ci Please test

natecook1000 · 2021-04-07T19:51:59Z

Looks great, @LemonSpike — thanks for this addition! 🎉

LemonSpike added 3 commits March 14, 2021 13:27

[test] Divide up existing Cycle tests and add FiniteCycle placeholder…

c636b09

… tests.

Implement FiniteCycle with Iterator wrapper around Product2 Iterator.

b4d0636

Change method signature of cycled(times:) to use new FiniteCycle.

70a2c56

LemonSpike force-pushed the finite_cycle_cycled branch from 63438b9 to 70a2c56 Compare March 14, 2021 13:27

Add tuple labels to Product2 for clarity.

3488868

xwu reviewed Mar 14, 2021

View reviewed changes

LemonSpike added 2 commits March 14, 2021 18:14

Apply column 80 width consistently.

a38d07a

Make base and times internal to avoid future Equatable issues.

fc2cdc1

LemonSpike marked this pull request as ready for review March 16, 2021 10:35

LemonSpike added 2 commits March 20, 2021 17:38

Refactor times and base to single Product2 type, and refactor Iterator.

4c4e36f

Add Collection, BidrectionalCollection and RandomAccessCollection con…

9f63577

…formance to FiniteCycle.

LemonSpike added 2 commits March 20, 2021 17:58

Add tests for index methods on FiniteCycle.

c257737

Add new count property to cycle with a test.

d876f8b

timvermeulen added the source breaking This change affects existing source code label Mar 23, 2021

LemonSpike added 4 commits March 24, 2021 11:18

Add @inlinable annotations.

037b09a

Remove Sequence conformance for FiniteCycle and add LazyCollectionPro…

154f738

…tocol conformance.

Update Cycle.md.

952afb0

Add conditional conformances for Equatable and Hashable.

62abcb4

Make more @inlinable.

6073193

timvermeulen reviewed Mar 31, 2021

View reviewed changes

LemonSpike added 3 commits April 5, 2021 15:26

Refactor typealias Index into struct, and remove Iterator.

f65f130

Make let product internal with @usableFromInline.

5527e1d

Revert tuple labels.

66b6aff

Update documentation descriptions.

e583de9

LemonSpike force-pushed the finite_cycle_cycled branch from cb3aba8 to e583de9 Compare April 7, 2021 10:48

natecook1000 merged commit bcdbac8 into apple:main Apr 7, 2021

LemonSpike deleted the finite_cycle_cycled branch April 16, 2021 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `FiniteCycle` type as the return type of `Collection.cycled(times:)` #106

Add new `FiniteCycle` type as the return type of `Collection.cycled(times:)` #106

LemonSpike commented Mar 14, 2021 •

edited

Loading

xwu left a comment

xwu Mar 14, 2021

xwu Mar 14, 2021

timvermeulen commented Mar 18, 2021

LemonSpike commented Mar 20, 2021 •

edited

Loading

timvermeulen commented Mar 22, 2021

LemonSpike commented Mar 24, 2021 •

edited

Loading

timvermeulen commented Mar 31, 2021

timvermeulen Mar 31, 2021

LemonSpike Apr 5, 2021

timvermeulen Apr 5, 2021

natecook1000 commented Apr 7, 2021

LemonSpike commented Apr 7, 2021

natecook1000 commented Apr 7, 2021

natecook1000 commented Apr 7, 2021

		@@ -56,6 +56,47 @@ extension Cycle: Sequence {

		extension Cycle: LazySequenceProtocol where Base: LazySequenceProtocol {}


		/// A collection wrapper that repeats the elements of a base collection for a finite number of times.

		public mutating func next() -> (element1: Base1.Element,
		element2: Base2.Element)? {

Add new FiniteCycle type as the return type of Collection.cycled(times:) #106

Add new FiniteCycle type as the return type of Collection.cycled(times:) #106

Conversation

LemonSpike commented Mar 14, 2021 • edited Loading

Checklist

xwu left a comment

Choose a reason for hiding this comment

xwu Mar 14, 2021

Choose a reason for hiding this comment

xwu Mar 14, 2021

Choose a reason for hiding this comment

timvermeulen commented Mar 18, 2021

LemonSpike commented Mar 20, 2021 • edited Loading

timvermeulen commented Mar 22, 2021

LemonSpike commented Mar 24, 2021 • edited Loading

timvermeulen commented Mar 31, 2021

timvermeulen Mar 31, 2021

Choose a reason for hiding this comment

LemonSpike Apr 5, 2021

Choose a reason for hiding this comment

timvermeulen Apr 5, 2021

Choose a reason for hiding this comment

natecook1000 commented Apr 7, 2021

LemonSpike commented Apr 7, 2021

natecook1000 commented Apr 7, 2021

natecook1000 commented Apr 7, 2021

Add new `FiniteCycle` type as the return type of `Collection.cycled(times:)` #106

Add new `FiniteCycle` type as the return type of `Collection.cycled(times:)` #106

LemonSpike commented Mar 14, 2021 •

edited

Loading

LemonSpike commented Mar 20, 2021 •

edited

Loading

LemonSpike commented Mar 24, 2021 •

edited

Loading