[Compacted] Adding compacted() method to remove all nils in a sequence or collection
#112
Conversation
LucianoPAlmeida
force-pushed
the
compact
branch
2 times, most recently
from
2d63197 to
b50ed7e
Compare
LucianoPAlmeida
force-pushed
the
compact
branch
from
b50ed7e to
f9da719
Compare
There was a problem hiding this comment.
Thanks for taking this one on, @LucianoPAlmeida! 👏 A few notes below about indexes and conformances.
|
@natecook1000 @kylemacomber Thank you for the review :) |
LucianoPAlmeida
force-pushed
the
compact
branch
6 times, most recently
from
f7d73f7 to
e446d44
Compare
LucianoPAlmeida
requested review from
natecook1000,
timvermeulen and
kylemacomber
There was a problem hiding this comment.
Looking good, @LucianoPAlmeida! Some more notes for you below, then I think we just need docs on the methods themselves.
LucianoPAlmeida
force-pushed
the
compact
branch
from
ff8b573 to
d6bf57d
Compare
…ce as convenience for .compactMap { $0 }
LucianoPAlmeida
force-pushed
the
compact
branch
from
89692d8 to
db76f21
Compare
LucianoPAlmeida
force-pushed
the
compact
branch
from
db76f21 to
abe4890
Compare
|
@natecook1000 Thanks again and sorry for those mistakes, I think all things were addressed =] |
|
@swift-ci Please test |
Sources/Algorithms/Compacted.swift
Outdated
| extension CompactedCollection: RandomAccessCollection | ||
| where Base: RandomAccessCollection {} |
There was a problem hiding this comment.
I missed this — because we don't know how far apart the non-nil elements are until we iterate, we can't make CompactedCollection conform to RandomAccessCollection.
There was a problem hiding this comment.
Makes sense :)
|
|
||
| func testCollectionTraversals() { | ||
| for array in self.tests { | ||
| validateIndexTraversals(array) |
There was a problem hiding this comment.
Looks like we need to call compacted() here.
There was a problem hiding this comment.
Sorry for that
| func testCollectionEquatableConformances() { | ||
| for array in self.tests { | ||
| XCTAssertEqual( | ||
| array.eraseToAnyHashableSequence().compacted(), | ||
| array.compactMap({ $0 }).eraseToAnyHashableSequence().compacted() | ||
| ) | ||
| XCTAssertEqual( | ||
| array.compacted(), array.compactMap({ $0 }).compacted() | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| func testCollectionHashableConformances() { | ||
| for array in self.tests { | ||
| let seq = array.eraseToAnyHashableSequence() | ||
| XCTAssertEqualHashValue( | ||
| seq.compacted(), seq.compactMap({ $0 }).compacted() | ||
| ) | ||
| XCTAssertEqualHashValue( | ||
| array.compacted(), array.compactMap({ $0 }).compacted() | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
For these two tests it would be great to make sure that equality and hashing work for non-equal base sequences/collections that are equal when compacted. E.g. [1, 2, 3, nil, nil, 4].compacted() == [1, nil, 2, nil, 3, 4].compacted(). You're covering the correctness of compacting already, so I don't think you need to do the comparison with compactMap { $0 } any more.
There was a problem hiding this comment.
Adjusted :)
LucianoPAlmeida
force-pushed
the
compact
branch
from
d301172 to
44ee039
Compare
LucianoPAlmeida
force-pushed
the
compact
branch
3 times, most recently
from
6aebc1f to
3db1df0
Compare
Sources/Algorithms/Compacted.swift
Outdated
|
|
||
| @inlinable | ||
| public func index(after i: Index) -> Index { | ||
| precondition(i < endIndex, "Index out of bounds") |
There was a problem hiding this comment.
| precondition(i < endIndex, "Index out of bounds") | |
| precondition(i != endIndex, "Index out of bounds") |
We try to compare indices using != wherever possible to be consistent with the stdlib, also see #65 (comment)
There was a problem hiding this comment.
Ah ok, thanks
|
@swift-ci Please test |
|
@swift-ci Please test |
|
|
Fixes #107
From the sketch from @kylemacomber on the issue, this just complement the implementation adding the tests and docs.
Let me know what you think =]
cc @kylemacomber @natecook1000 @timvermeulen
Replace this paragraph with a description of your changes and rationale. Provide links to an existing issue or external references/discussions, if appropriate.
Checklist