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.

Sign up for GitHub

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

Jump to bottom

feat(SourceT): add fromConsumableActionStep that creates a StepT from… #1533

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(SourceT): add fromConsumableActionStep that creates a StepT from… #1533

wants to merge 1 commit into from

Conversation

giulioforesto
Copy link

… an action that consumes some data

Closes #1448

alpmestan
alpmestan requested changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@alpmestan alpmestan left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

LGTM, just a little bit of nitpicking.

changelog.d/generalize-fromActionStep-to-consumable Outdated Show resolved Hide resolved
servant/src/Servant/Types/SourceT.hs Outdated Show resolved Hide resolved
servant/src/Servant/Types/SourceT.hs Outdated Show resolved Hide resolved
servant/src/Servant/Types/SourceT.hs Show resolved Hide resolved
@giulioforesto giulioforesto force-pushed the generalize-fromActionStep-to-consumable branch 4 times, most recently from 0725a12 to bf28f64 Compare February 17, 2022 16:18
alpmestan
alpmestan approved these changes Feb 18, 2022
View reviewed changes
Copy link
Contributor

@alpmestan alpmestan left a comment

Choose a reason for hiding this comment

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

This seems ready to land, modulo the hardest part: the name of your function :-)

I think I'm warming up to unfoldStep.

@giulioforesto
Copy link
Author

This seems ready to land, modulo the hardest part: the name of your function :-)

I think I'm warming up to unfoldStep.

Haha go for unfoldStep then. I trust you, I don't master all the subtleties of Haskell function naming yet ;)

@giulioforesto giulioforesto force-pushed the generalize-fromActionStep-to-consumable branch from bf28f64 to 2947532 Compare February 18, 2022 18:14
@giulioforesto
Copy link
Author

There you go, thanks for the review

@ysangkok
Copy link
Contributor

@giulioforesto The doctest isn't passing for me. Was this only meant as a hint/suggestion, or was it supposed to be actually be an executable doctest?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alpmestan alpmestan alpmestan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make fromAction and fromActionStep able to pass a value from an action to the next one
3 participants
@giulioforesto @ysangkok @alpmestan