Closed LemonSpike closed 3 years ago
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?
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. 😊
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:
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.@inlinable
annotations.Cycle.md
guide needs a slight tweak to reflect that cycled(times:)
now has a new return type.Hi @timvermeulen, thanks for the feedback. Re: your points, I tried to implement your suggestions as follows:
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.
FiniteCycle.Index
is a typealias for Product2.Index
, the Index
Hashable
conformance was already synthesized.@inlinable
to all public methods, by making the let product
public
.despite its name
since the name no longer includes Sequence
.@usableFromInline
internal
methods to @inlinable
because of #109.Let me know your thoughts, thanks.
Apologies for the delay, @LemonSpike!
- I removed the
Sequence
conformance entirely. To conform toLazyCollectionProtocol
I had to conform toLazySequenceProtocol
andLazyCollectionProtocol
together whereBase: LazyCollectionProtocol
, because I couldn't inheritLazySequenceProtocol
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 forProduct2.Index
, theIndex
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 thelet 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 includesSequence
.
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 #109.
💯
@swift-ci Please test
@swift-ci Please test
@swift-ci Please test
Looks great, @LemonSpike — thanks for this addition! 🎉
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 toProduct2
when using theIterator.next()
method and also in thesubscript
method. This is a cosmetic improvement, but I felt it may read better.Checklist