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

Relocate Enter to `servant` package #478

Merged
merged 6 commits into from May 10, 2016

Conversation

Projects
None yet
5 participants
@amarpotghan
Copy link
Contributor

commented Apr 20, 2016

@soenkehahn and I were pairing on implementing staticClient function which will allow providing BaseUrl (or any argument in general) statically. After considering a few approaches we realised that Enter is already capable of achieving that (with an additional instance). Hence we decided to move Enter to servant package so that we could use it in servant-client without depending on servant-server.

Please let us know your thoughts.

@jkarni jkarni added the in progress label Apr 20, 2016

import Servant.Server
import Servant.Server.Experimental.Auth
import qualified Servant.Common.Req as SCR

This comment has been minimized.

Copy link
@soenkehahn

soenkehahn Apr 20, 2016

Contributor

Above is lots of whitespace change. Can you remove that from the PR, please?

This comment has been minimized.

Copy link
@amarpotghan

amarpotghan Apr 20, 2016

Author Contributor

Sure, will get rid of those

:<|> (_ :: String -> Maybe Int -> Bool -> [(String, [Rational])] -> C.Manager -> SCR.ClientM (String, Maybe Int, Bool, [(String, [Rational])]))
:<|> (_ :: C.Manager -> SCR.ClientM (Headers TestHeaders Bool))
:<|> (_ :: C.Manager -> SCR.ClientM NoContent) = staticClient baseUrl api
(left show <$> runExceptT (getGet' manager)) `shouldReturn` Right alice

This comment has been minimized.

Copy link
@soenkehahn

soenkehahn Apr 20, 2016

Contributor

So this only works if we provide all the types? It's not enough to provide the type of getGet'?

This comment has been minimized.

Copy link
@amarpotghan

amarpotghan Apr 20, 2016

Author Contributor

Yeah unfortunately full type is required here.

This comment has been minimized.

Copy link
@jkarni

jkarni Apr 20, 2016

Member

In theory that's not the case though. Is it the case here because Enter is missing a fundep? (Which may not be fixable though - there were some issues with a sensible fundep on GHC 7.6).

This comment has been minimized.

Copy link
@amarpotghan

amarpotghan Apr 20, 2016

Author Contributor

Ah, I overlooked it, I was still thinking about [HasArgument](https://github.com/haskell-servant/servant/blob/mappable/servant/src/Servant/Utils/Map.hs) class in my head :). Enter already has required fundep, it should work without full type. Will try that tomorrow morning.

-- > postNewBook' :: Book -> Manager -> ClientM Book
-- > (getAllBooks' :<|> postNewBook') = staticClient (BaseUrl Http "localhost" 8080 "/") myApi
staticClient :: (Enter (Client layout) BaseUrl ret, HasClient layout) => BaseUrl -> Proxy layout -> ret
staticClient b = enter b . client

This comment has been minimized.

Copy link
@soenkehahn

soenkehahn Apr 20, 2016

Contributor

I'm not sure if staticClient is really the best name. Does anyone have better ideas?

@@ -39,6 +39,9 @@ instance ( Enter typ1 arg1 ret1, Enter typ2 arg2 ret2
instance (Enter b arg ret) => Enter (a -> b) arg (a -> ret) where
enter arg f a = enter arg (f a)

instance Enter (arg -> ret) arg ret where
enter arg f = f arg

This comment has been minimized.

Copy link
@soenkehahn

soenkehahn Apr 20, 2016

Contributor

It feels a bit arbitrary to have an instance like this. Maybe it's better to have some newtype wrapper for the argument?

instance Enter (arg -> ret) (InjectedArgument arg) ret where
  enter (InjectedArgument arg) f = f arg

This comment has been minimized.

Copy link
@amarpotghan

amarpotghan Apr 20, 2016

Author Contributor

Yeah that's a good idea, will change it

This comment has been minimized.

Copy link
@jkarni

jkarni Apr 20, 2016

Member

Hm, this still seems like it muddles the purpose of enter. enter should apply a natural transformation to the return types of functions combined with :<|>. I don't think it should take on extra burdens, unless we can very clearly state it's new purpose (without using a conjuction or disjunction in the explanation). There are a lot of similar transformations I can imagine, and if we start including them all as instances for enter without a good overarching story, it'll become pretty hard to understand or explain what enter does.

I think I myself would prefer code that looks a lot like enter but just for what we want to do in the client. It's not like there'd be much duplicated code. Conceptually, if I'm getting this right, we need only two instances - one for recursing over :<|> and one for applying the client functions to the baseurl/manager arg.

This comment has been minimized.

Copy link
@amarpotghan

amarpotghan Apr 20, 2016

Author Contributor

I don't have strong opinion about this. Just that, we thought we were (almost) re-inventing Enter with our HasArgument class, but I get your point of doing more than one thing without sensible laws would be vague. I am all in for replicating Enterlike class for staticClient purpose. WDYT @soenkehahn ?

This comment has been minimized.

Copy link
@soenkehahn

soenkehahn Apr 21, 2016

Contributor

Yes, I think that's better. I wouldn't include it in servant then, though.

@jkarni: @amarpotghan and I tried for some time to come up with a nice general way of mapping over the leafs of a :<|>-:>-structure, but didn't manage. We hoped that this general mapping function would also allow to implement staticClient. Now since it's much less general, I think that this mapping should stay in servant-client.

(We could still move enter to servant, since it's been proven to be useful in e.g. servant-client, but that'd be a completely unrelated change. #NoScopeCreep)

@amarpotghan amarpotghan force-pushed the enter-relocation branch 3 times, most recently from 663e4aa to a66300a Apr 21, 2016

@amarpotghan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2016

@soenkehahn @jkarni : I removed staticClient stuff from this PR. It only contains moving Enter from servant-server to servant. I will create an another PR for staticClient later

@amarpotghan amarpotghan changed the title Relocate Enter to `servant` package and implement a `staticClient` function for static arguments Relocate Enter to `servant` package Apr 21, 2016

@jkarni

This comment has been minimized.

Copy link
Member

commented Apr 22, 2016

I'm in favour of this. FWIW, I thought a little more about the general problem, and I think an option is making the return type of the client function a Reader monad over BaseUrl and Manager. Then enter would work without modification for this purpose.

(There are merge conflicts from the commits here that have already been merged)

@3noch

This comment has been minimized.

Copy link

commented Apr 22, 2016

@jkarni Could you have an analog to enter for client functions? I.e. you provide a transformation from the client function result to some user-defined result type. Then anyone could construct such a Reader version of their client functions themselves.

@jkarni

This comment has been minimized.

Copy link
Member

commented Apr 22, 2016

@3noch enter itself should work as is for client functions (which is a large part of why this relocation makes sense)

@3noch

This comment has been minimized.

Copy link

commented Apr 22, 2016

@jkarni Oh, awesome!

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2016

Can't we remove the mmorph dep from servant-server then? Also, this requires updating the tutorial, the section about enter (for the import). Apart from that, LGTM!

@amarpotghan amarpotghan force-pushed the enter-relocation branch from 8fe9db3 to 845a06c Apr 28, 2016

@amarpotghan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2016

removed the mmorph from servant-server. I couldn't find any Enter import
in doc though.

On Thu, Apr 28, 2016 at 9:56 PM, Alp Mestanogullari <
notifications@github.com> wrote:

Can't we remove the mmorph dep from servant-server then? Also, this
requires updating the tutorial, the section about enter (for the import).
Apart from that, LGTM!


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#478 (comment)

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2016

Nevermind then. LGTM.

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2016

Oh hang on, do we want to export Servant.Utils.Enter from some "core" module? Or should users just import it when they need it?

@amarpotghan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2016

actually Nat, enter and Nat utils are reexported from Servant.Server, exporting them from Servant would make more sense?

@jkarni

This comment has been minimized.

Copy link
Member

commented May 10, 2016

I'm okay leaving the export situation as is for now. When the uses for enter in other situations are worked out, we can export it from servant.

@jkarni

This comment has been minimized.

Copy link
Member

commented May 10, 2016

LGTM

@jkarni

This comment has been minimized.

Copy link
Member

commented May 10, 2016

Oh, could you add the change to the changelogs?

@amarpotghan

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2016

@soenkehahn @jkarni I tentatively put changelogs under 0.7.1, not sure if that's the correct version?

@jkarni

This comment has been minimized.

Copy link
Member

commented May 10, 2016

@amarpotghan yup - assuming we merge in time for the release (#499).

@jkarni

This comment has been minimized.

Copy link
Member

commented May 10, 2016

@alpmestan not clear whether your LGTM still holds?

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented May 10, 2016

Yeah, nevermind my comment about exports, this can be tweaked later, let's not block this patch on some never-ending discussion about the optimal way to export enter =)

@jkarni jkarni merged commit 1955c5a into master May 10, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jkarni jkarni removed the in progress label May 10, 2016

@jkarni jkarni deleted the enter-relocation branch May 10, 2016

@jkarni

This comment has been minimized.

Copy link
Member

commented May 10, 2016

@amarpotghan thanks!

@soenkehahn

This comment has been minimized.

Copy link
Contributor

commented May 11, 2016

Great, thanks @amarpotghan!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.