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 Source.mapStateful and Source.mapStatefulConcat combinators #15

Merged
merged 4 commits into from
Oct 17, 2023

Conversation

rucek
Copy link
Contributor

@rucek rucek commented Oct 6, 2023

I'm not sure whether f should return (S, Option[U]) or (S, U) - where in the former case the Option indicates whether anything should be sent downstream.

The former is more flexible, but introduces additional complexity to the signature. Comparing to Akka Streams, the current approach mixes statefulMap and statefulMapConcat a bit, in that you can control whether the result of f will be sent downstream at all. However, unlike in Akka Streams, f can only return an Option, not an Iterable (which is flattened in Akka's statefulMapConcat).

Any thoughts on this (or anything else in the proposed approach)?

@rucek rucek requested a review from adamw October 6, 2023 16:32
@rucek
Copy link
Contributor Author

rucek commented Oct 6, 2023

Also, any ideas for more test cases are welcome 😄

@adamw
Copy link
Member

adamw commented Oct 10, 2023

I think that the akka streams approach might make sense - a general mapStatefulConcat, and a special-case, simplified mapStateful?

@rucek rucek changed the title Add Source.statefulMap combinator Add Source.mapStateful and Source.mapStatefulConcat combinators Oct 12, 2023
@rucek
Copy link
Contributor Author

rucek commented Oct 12, 2023

See #18 (comment) for a discussion about checking isValue after send().

false
case ChannelClosed.Error(r) =>
c.error(r)
false
Copy link
Member

Choose a reason for hiding this comment

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

I think here we already have the desired error handling, so no changes should be necessary :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're still checking the result of c.send below in result.iterator.map(c.send).forall(_.isValue), although we assume that send should not fail since we control c, so we should probably always return true after sending, right?

Copy link
Member

Choose a reason for hiding this comment

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

right, though this at worst is dead code - not that there's a possiblity of incorrect behavior. We can change this though:

  • ignoring the return value probably needs a comment that we know what we are doing - and why
  • in larger code chunks (which are harder to follow), we might opt for the defensive solution anyway

@adamw adamw merged commit 02f5198 into master Oct 17, 2023
4 checks passed
@adamw adamw deleted the source-stateful-map branch February 21, 2024 15:21
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