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

servant-client: Run ClientEnv's makeClientRequest in IO #1595

Merged
merged 2 commits into from Jul 1, 2022

Conversation

Minnozz
Copy link
Contributor

@Minnozz Minnozz commented Jun 17, 2022

Use cases:

@tchoutri
Copy link
Contributor

@Minnozz Hi! Has there been a discussion about this PR? :)

@Minnozz
Copy link
Contributor Author

Minnozz commented Jun 17, 2022

@tchoutri Hi! This change was suggested by alp__ on IRC today in response to a question from me. I took a shot at implementing it; should it also be discussed elsewhere?

@tchoutri
Copy link
Contributor

Great, then I'll leave @alpmestan review your PR. :)

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 looks fine to me. We'll want to add an entry in the relevant changelog. Also, this will force a major version bump.

Let's perhaps leave a few days to give others a chance to give feedback on this, and for you to add a changelog entry (see how other PRs do it, with entries in the changelog.d directory).

servant-client/test/Servant/SuccessSpec.hs Show resolved Hide resolved
@Minnozz
Copy link
Contributor Author

Minnozz commented Jun 21, 2022

@alpmestan I added a changelog entry, do you think it needs more explanation?

@alpmestan
Copy link
Contributor

@Minnozz Should be fine, I think people using servant are familiar enough with IO =)

If you ever feel like writing down a little something that demonstrates how you're using this to your advantage (cookbook recipe, blog post, whatever), then it can serve as a reference for users who might need this in the future. But that's more work that adding a few lines in the changelog :-)

@tchoutri
Copy link
Contributor

tchoutri commented Jul 1, 2022

Considering the approval of this PR, I will merge, but a cookbook or a blog post would be very appreciated, @Minnozz :)

@tchoutri tchoutri merged commit 489cbd5 into haskell-servant:master Jul 1, 2022
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.

None yet

3 participants