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 alsoTo/alsoToContext to FlowWithContext/SourceWithContext #1443

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Aug 22, 2024

This PR adds alsoTo/alsoToContext onto FlowWithContext/SourceWithContext which just wrap the underlying delegate methods, this is analogous to the current map/mapContext methods that already exist.

Note that delegate.alsoTo(Flow.fromFunction { in: (Out, Ctx) => in._2 }.toMat(that)(Keep.right)) is copied from the implementation of contramap. I would have ideally liked to use contramap however the method is not available on a Graph[SinkShape[_]]

I didn't add tests due to the sheer triviality of the implementation but can add them if asked.

@mdedetrich mdedetrich force-pushed the add-alsoTo-alsoToContext-to-Flow-Source-with-context branch 3 times, most recently from 1e091bb to facf742 Compare August 22, 2024 17:24
@mdedetrich
Copy link
Contributor Author

PR is now ready to review, the link-validator failure is not related

@pjfanning
Copy link
Contributor

pjfanning commented Aug 22, 2024

I would like a couple of trivial test cases - even if for regression purposes. Also useful as a way of documenting the usage.

@mdedetrich mdedetrich force-pushed the add-alsoTo-alsoToContext-to-Flow-Source-with-context branch 8 times, most recently from 6ed64a7 to ad0c4e5 Compare August 23, 2024 12:12
Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm

@mdedetrich mdedetrich force-pushed the add-alsoTo-alsoToContext-to-Flow-Source-with-context branch from ad0c4e5 to 70d929b Compare August 23, 2024 13:18
@mdedetrich
Copy link
Contributor Author

Thanks for the review, everything is passing and I have added the @since, merging.

@mdedetrich mdedetrich merged commit 302f38f into apache:main Aug 23, 2024
19 checks passed
@mdedetrich mdedetrich deleted the add-alsoTo-alsoToContext-to-Flow-Source-with-context branch August 23, 2024 13:56
@pjfanning pjfanning added this to the 1.1.0 milestone Aug 23, 2024
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.

2 participants