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

Add recursiveMap(_:) methods #118

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Conversation

SusanDoggie
Copy link

async version of apple/swift-algorithms#185

@SusanDoggie SusanDoggie marked this pull request as ready for review March 28, 2022 03:46
@SusanDoggie SusanDoggie changed the title add recursiveMap methods add recursiveMap(_:) methods Mar 28, 2022
var base: Base.AsyncIterator?

@usableFromInline
var mapped: ArraySlice<Transformed> = []
Copy link
Member

Choose a reason for hiding this comment

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

so what happens if the base is indefinite? does this potentially grow with out bound?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it’s definitely problems. Maybe I should change to depth first algorithm and it shouldn’t have indefinite depth of tree hopefully.

Copy link
Contributor

@kingreza kingreza Mar 28, 2022

Choose a reason for hiding this comment

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

This is pretty cool!

I have a somewhat and open-ended question, I believe using depth first would change the expected output in the example above from 1,2,3,4,5,6 to 1,3,4,6,5,2. The traversal would still be correct but given that the output varies so widely depending on the type of traversal, should that be communicated in the function name/signature? Or is order not that important in this scenario?

Is the utility of this function its ability to visit each node once and iterate over the tree or do/should we care about the order in the sequence?

Copy link
Author

Choose a reason for hiding this comment

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

This function is a simplified version of SQL recursive CTE, I implemented breadth-first traversal because the form of the algorithm is already in my head.
It can be done by function name, or using a enum option for choosing the traversal methods.

) -> AsyncThrowingRecursiveMapSequence<Self, S>
}
```

Copy link
Member

Choose a reason for hiding this comment

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

it might be good here to give a bit more detail, particularly things that are helpful: how does it work with cancellation, identifying the Sensibility requirements, how do they interact with rethrows etc is good to record in the guides.

Also links to existing counterparts etc.

}

@inlinable
public mutating func next() async rethrows -> Base.Element? {
Copy link
Member

Choose a reason for hiding this comment

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

this seems to be breadth first recursion; would it perhaps make more sense in the async case to be depth first?

@preconcurrency import XCTest
import AsyncAlgorithms

final class TestRecursiveMap: XCTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

couple of key tests that are good to have: testing how cancellation works and testing going past the end of iteration. Testing throwing might be good here too.

I do kinda wonder what the validation diagrams might look for this, is that something we can do with this algorithm? or is there something more that needs to be added to those to get that to work?

while self.mapped_iterator != nil {

if let element = try await self.mapped_iterator?.next() {
try await mapped.append(transform(element))
Copy link
Member

Choose a reason for hiding this comment

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

if this throws, don't you have the iterator potentially be in a bad state? where it may throw an error for this call to next but then the next call to next violates the expectation that past a nil or a thrown error the iterators must return nil for subsequent calls to next.

@SusanDoggie SusanDoggie changed the title add recursiveMap(_:) methods [DNM] add recursiveMap(_:) methods Mar 28, 2022
@SusanDoggie SusanDoggie changed the title [DNM] add recursiveMap(_:) methods Add recursiveMap(_:) methods Mar 28, 2022
@SusanDoggie SusanDoggie requested a review from phausler March 31, 2022 00:56
@parkera parkera added the v1.1 Post-1.0 work label May 4, 2022
@parkera
Copy link
Member

parkera commented May 4, 2022

Marking as v1.1 because we should strongly consider this after our 1.0 API stability release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.1 Post-1.0 work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants