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

Integrate MultiVerb into the servant packages #1766

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

theophile-scrive
Copy link
Contributor

@theophile-scrive theophile-scrive commented Jul 1, 2024

This commit is Part 1 of the integration, where only the packages of this monorepo are touched.

MultiVerb is integrated alongside of the other Verb machinery, so that people may take their time to transition to it.


IsWaiBody will not be able to live in its current form, as it mixes concepts from the Server and Client packages.

This impacts SomeResponse, which itself impacts the HasClient and HasServer instances.
ResponseF can be replicated internally, even without the httpVersion field.
SomeResponse can be kept on the server, but the client can move to use ResponseF.


The IsResponse and IsResponseList classes are split between client-side Reponse{,List}Unrender and server-side Response{,List}Render.


This is a collaborative work between Scrive AB and Wire Swiss GmbH.

@theophile-scrive theophile-scrive force-pushed the integrate-multiverb branch 3 times, most recently from b476b8f to 84ccbd0 Compare July 8, 2024 12:35
@theophile-scrive theophile-scrive changed the title Integrate MultiVerb into servant package Integrate MultiVerb into the servant package Jul 10, 2024
@theophile-scrive theophile-scrive changed the title Integrate MultiVerb into the servant package Integrate MultiVerb into the servant packages Jul 10, 2024
@theophile-scrive theophile-scrive force-pushed the integrate-multiverb branch 6 times, most recently from d10fc87 to 4ecb4ec Compare August 9, 2024 09:44
@theophile-scrive theophile-scrive force-pushed the integrate-multiverb branch 2 times, most recently from b7e31f0 to 77d471f Compare August 27, 2024 13:56
@theophile-scrive theophile-scrive force-pushed the integrate-multiverb branch 2 times, most recently from a61130b to 046d35f Compare August 28, 2024 22:06
@theophile-scrive theophile-scrive force-pushed the integrate-multiverb branch 11 times, most recently from 8be000f to 1a6b30d Compare August 29, 2024 20:16
@theophile-scrive theophile-scrive force-pushed the integrate-multiverb branch 2 times, most recently from 6d3234e to 3a84e18 Compare September 12, 2024 12:51
@theophile-scrive theophile-scrive marked this pull request as ready for review September 12, 2024 12:51
@theophile-scrive
Copy link
Contributor Author

@fisx @ysangkok I'd appreciate a review or questions if some things are not clear!

Copy link
Member

@fisx fisx left a comment

Choose a reason for hiding this comment

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

sorry for dropping the ball. here are some more comments. i'll continue with the review soon, i promise! :)

doc/cookbook/generic/Generic.lhs Show resolved Hide resolved
doc/cookbook/multiverb/MultiVerb.lhs Show resolved Hide resolved
doc/cookbook/multiverb/MultiVerb.lhs Outdated Show resolved Hide resolved
servant/src/Servant/API/MultiVerb.hs Show resolved Hide resolved
Copy link
Member

@fisx fisx left a comment

Choose a reason for hiding this comment

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

Sorry for the lag, but I think we're getting there!

Could you go through the unresolved comments one more time and confirm that you're ok with resolving all of them?

If so I'll approve this PR.

After that: @arianvp wanna give it a read, and maybe merge? @pcapriotti as the author of this code, do you have any feedback?

doc/cookbook/multiverb/MultiVerb.lhs Show resolved Hide resolved
servant/src/Servant/API/MultiVerb.hs Outdated Show resolved Hide resolved
servant/src/Servant/API/MultiVerb.hs Outdated Show resolved Hide resolved
servant-client/test/Servant/ClientTestUtils.hs Outdated Show resolved Hide resolved
@theophile-scrive
Copy link
Contributor Author

@pcapriotti @fisx I'd love your final feedback on this. :)

Copy link
Member

@jkarni jkarni left a comment

Choose a reason for hiding this comment

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

Nice feature, and very nice to read PR!

I made some comments, but none of them should be considered blocking!

doc/cookbook/multiverb/MultiVerb.lhs Show resolved Hide resolved
servant-client/test/Servant/ClientTestUtils.hs Outdated Show resolved Hide resolved
doc/cookbook/multiverb/MultiVerb.lhs Outdated Show resolved Hide resolved
servant/src/Servant/API/MultiVerb.hs Outdated Show resolved Hide resolved
servant/src/Servant/Types/ResponseList.hs Outdated Show resolved Hide resolved
servant/src/Servant/API/TypeLevel/List.hs Show resolved Hide resolved
Copy link
Member

@fisx fisx left a comment

Choose a reason for hiding this comment

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

sorry for taking so incredibly long! 😳

@theophile-scrive theophile-scrive force-pushed the integrate-multiverb branch 3 times, most recently from a230b7d to 5a88909 Compare January 14, 2025 12:19
This commit is Part 1 of the integration, where only the
`servant`Epackage is touched. `Verb` is redefined as an alias for
`MultiVerb1` inEorder to make the transition transparent to users of
`Verb`.
@theophile-scrive
Copy link
Contributor Author

Very well, with this last input, I think we are good to merge.

@theophile-scrive
Copy link
Contributor Author

@fisx would you do me the honour?

-- | This module offers other servant libraries a minimalistic HTTP response type.
--
-- It is purely an internal API and SHOULD NOT be used by end-users of Servant.
module Servant.Types.Internal.ResponseList where
Copy link
Member

Choose a reason for hiding this comment

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

why is it called ResponseList?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it used to probably host stuff in relation with IsResponseList but does not anymore. I'll rename it to Response.

@fisx fisx merged commit 4494de0 into haskell-servant:master Jan 16, 2025
6 checks passed
@fisx
Copy link
Member

fisx commented Jan 16, 2025

@fisx would you do me the honour?

merged. also the honor is all mine :-) thanks for your infinite patience!

@fisx
Copy link
Member

fisx commented Jan 16, 2025

@phadej @jkarni @...

any objections to me cutting a new release tomorrow? (come to think of it, should i be on the google mailing list so i can ask these things there? i remember vaguely having been opposed to gmail when that was started, but my opposition appearently didn't make google go away... :)

@tchoutri
Copy link
Contributor

I'd like us to take some time to address some of the tickets that have appeared since the last release before.

@tchoutri
Copy link
Contributor

Moreover I'd also be comfortable with advertising the MultiVerb feature first so that people may try it before we cut a release.

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.

7 participants