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

Update README using new API #10

Closed
wants to merge 9 commits into from
Closed

Update README using new API #10

wants to merge 9 commits into from

Conversation

AphonicChaos
Copy link
Contributor

This is not ready. I just wanted something that was easy to reference when discussing the changes.

Progress

  • update code snippets to match API updates
  • reword tutorial sections such that they also match the API updates
  • determine which important bits of the API are missing coverage (such as the new status code functionality)

@AphonicChaos
Copy link
Contributor Author

I suppose had I given it a bit more thought, I'd have started with a PR instead of an issue to track this. I guess part of my rational there was I wasn't sure how much work it would be to update the documentation, and thus whether I'd be the right person to do so.

Luckily, @tel, it so happens that learning about Serv has been fun for me, in particular because I have a project which is currently (happily) using Servant which would make for a perfect test bed in comparing approaches.

@AphonicChaos
Copy link
Contributor Author

@tel I was just about to update this section of the tutorial:

> :kind Impl Api_All IO
Impl Api_All IO :: *
= (IO (Response '[] 'Empty)
   :<|> (IO (Response '['H.Location '::: RawText] 'Empty)
         :<|> (IO (Response '[] ('Body '[Ct.TextPlain] Text))
               :<|> IO NotHere)))
  :<|> ((Tagged "user_id" Int
         -> IO (Response '[] 'Empty)
            :<|> (IO (Response '['H.Location '::: RawText] 'Empty)
                  :<|> (IO (Response '[] ('Body '[Ct.TextPlain] Text))
                        :<|> IO NotHere)))
        :<|> (([Text]
               -> IO (Response '[] 'Empty)
                  :<|> (IO (Response '['H.Location '::: RawText] 'Empty)
                        :<|> (IO (Response '[] ('Body '[Ct.TextPlain] Text))
                              :<|> IO NotHere)))
              :<|> IO NotHere))

When I followed along with the updated code and, to my surprise, got this instead:

> :k Impl IO Api_All
Impl IO Api_All :: *

I assume this is due to all of the Singleton loveliness, and thus expected. The question then becomes: Can I just remove this part of the tutorial?

I just did the same thing with branch 4b966ad (which is from before you implemented singletons) and am getting the same simple kind. Looking at the output I see that's the same as well, so I don't understand how you're getting the additional output.

Figured it out. You're using :kind!, not !kind.

@tel
Copy link
Owner

tel commented Feb 7, 2016

Ah, good catch! I left out the ! apparently.

@AphonicChaos
Copy link
Contributor Author

I need a break from this. it's not clear to me from the example how someone would transition from the old API to the new one. In particular, the readme currently has:

type Impl_Endpoint =
       IO (Response '[] 'Empty)
  :<|> IO (Response '['H.Location '::: RawText] 'Empty)
  :<|> IO (Response '[] ('Body '[Ct.TextPlain] Text))
  :<|> IO NotHere

type Impl_All =
       Impl_Endpoint
  :<|> Tagged "user_id" Int -> Impl_Endpoint
  :<|> [Text] -> Impl_Endpoint
  :<|> IO NotHere

server :: Server IO
server = handle (Proxy :: Proxy Api_All) (myImplementation :: Impl_All)

But the example shows:

apiSing :: Sing TheApi
apiSing = sing

impl :: Impl IO TheApi
impl = get :<|> put :<|> delete :<|> MethodNotAllowed
  where
    get =
      respond
      $ emptyResponse Sc.SOk
      & withHeader H.SCacheControl "foo"
      & withBody "Hello"
    put =
      respond
      $ emptyResponse Sc.SOk
      & withHeader H.SCacheControl "foo"
      & withBody [1, 2, 3]
    delete =
      respond
      $ emptyResponse Sc.SInternalServerError
      & withBody "Server error"


theServer :: Server IO
theServer = server apiSing impl

I was able to come up with this:

type Impl_Endpoint_ = 
       IO (Response Sc.Ok '[] Empty)
  :<|> IO (Response Sc.Ok '[ H.Location ::: RawText ] Empty)
  :<|> IO (Response Sc.Ok '[] RawBody)
  :<|> IO NotFound

type Impl_All =
       Impl_Endpoint_
  :<|> Tagged "user_id" Int -> Impl_Endpoint_
  :<|> [Text] -> Impl_Endpoint_
  :<|> IO NotFound

As you see, I added an extra underscore to Impl_Endpoint since you seem to be using Impl_Endpoint internally. I get stuck here, however, because handle doesn't seem to be exposed. I suspect that most of this should now be unnecessary as because of a) the aforementioned name conflicts and b) the example doesn't use this at all. However, I was unable to figure out how to write and impl function for the Api_All type defined in the readme.

I suspect that once that's figured out, that entire part of the readme will need to be rewritten. If you could clear up the above confusion for me, I'll probably take another stab at it late this evening or tomorrow. Is it stands now, I've already spent several hours on what began as a simple typo correction, so I'm a bit burnt out at the moment, sorry.

@tel
Copy link
Owner

tel commented Feb 7, 2016

Apologies for the burnout—feel free to reach out to me as soon as you can with questions so that I can hopefully help avoid such problems.

@AphonicChaos AphonicChaos changed the title Fixed indentation and updated a few types in the tutorial. Update README using new API Feb 7, 2016
@tel
Copy link
Owner

tel commented Feb 7, 2016

So, first the simple thing: handle has been renamed to server

server :: (Constrain api, Monad m) => Sing api -> Impl m api -> Server m

and through use of singletons is no longer a class member.

@AphonicChaos
Copy link
Contributor Author

Clarification on my previous comment would be a good start ;-). Specifically:

  • Are Impl_Endpoint_ and Impl_All still necessary with the new API?
  • Am I meant to just be able to write an implementation for Impl IO Api_All instead?
  • What does said implementation look like? I've tried a couple of times but get scary type errors that are beyond my comprehension given the fun kind magic you're doing.

@tel
Copy link
Owner

tel commented Feb 7, 2016

Second, the style of API definitions has changed. Here are some befores and their corresponding afters

-- before
type Method_1 = 'Method 'DELETE '[] 'Empty

-- after
type Method_1 = Method DELETE '[ Sc.NoContent ::: Respond '[] Empty ]

Changes include

  • no longer needing to "tick" the types since I export type aliases like type DELETE = 'DELETE.
  • Method now takes the method verb and a list of "response alternatives" which map status codes to header sets and bodies—I assume new information that we'll get back a 204 No Content response here.
-- before
type Endpoint_1 = 'Endpoint '[ Method_1, Method_2, Method_3 ]

-- after
type Endpoint_1 = Endpoint () '[ Method_1, Method_2, Method_3 ]

Changes include

  • No more "ticking" as before.
  • Endpoint takes an annotation argument which can be used later as a hook for documentation (this is not clearly the best way to do this, I am interested in feedback)

@tel
Copy link
Owner

tel commented Feb 7, 2016

To understand how Api types translate to Impl m types, you can just look at the type functions which transform them. In particular, Impl_Endpoint converts Endpoint types to their implementation types and it relies a lot on Impl_Handler which converts each method within the Endpoint. Perhaps most crucial to the new style is the mapping to SomeResponse which says that we need return "some" response from the list of alternatives.

SomeResponse lives here and is a standard dependently typed trick for representing a type which has membership inside of a list. A simpler example is this:

data Index (xs :: [*]) where
  Skip :: Index xs -> Index (x ': xs)
  Stop :: x -> Index (x ': xs)

type TypeList = [Char, Bool, Int, (), [Bool], String]

ex1 :: Index TypeList
ex1 = Skip (Skip (Stop 3))

ex2 :: Index TypeList
ex2 = Stop 'c'

ex3 :: Index TypeList
ex3 = Skip (Skip (Skip (Skip (Stop [True, False]))))

etc etc

@tel
Copy link
Owner

tel commented Feb 7, 2016

Here are some answers:

Are Impl_Endpoint_ and Impl_All still necessary with the new API?

They're not necessary. They just exist to illustrate what's going on. The new API uses Impl_Endpoint for the specialization of the Impl type family to Endpoints which is why you needed the extra underscore.

Am I meant to just be able to write an implementation for Impl IO Api_All instead?

You can do this, yep. You can also write implementations for pieces of Api_All and inference ought to work out.

What does said implementation look like?

An implementation of an Endpoint () [a, b, c] looks a bit like impl_a :<|> impl_b :<|> impl_c :<|> MethodNotAllowed. An implementation of Method Verb Alternatives looks a bit like respond alt for some alt valid under Alternatives. An implementation of OneOf [a, b, c] looks a bit like impl_a :<|> impl_b :<|> impl_c :<|> NotFound.

Does this help?

@AphonicChaos
Copy link
Contributor Author

@tel: Picked this back up again. I haven't gotten to your last comment yet, but what you've written so far has been rather illuminating. I don't have a firm grasp of type families or GADTs, but I think your examples make things clear enough. Thank you very much.

@AphonicChaos
Copy link
Contributor Author

@tel one last hurdle. I'm Having trouble creating a server for an implementation which includes captures. The following compiles:

{-# LANGUAGE DataKinds         #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE PolyKinds         #-}
{-# LANGUAGE TypeOperators     #-}
module Main where

import Data.Text (Text)
import Data.Function ((&))
import Serv.Api
import qualified Serv.ContentType as Ct
import qualified Serv.StatusCode as Sc

import Serv.Common (RawText)
import qualified Serv.Header as H

import Serv.Server
import qualified Network.Wai as Wai


type Method_1 = Method DELETE '[ Sc.NoContent ::: Respond '[] Empty ]

type Method_2 = Method POST 
                '[ Sc.NoContent ::: Respond '[ H.Location ::: RawText ] Empty ]

type RawBody = HasBody '[ Ct.TextPlain ] Text

type Method_3 = Method GET '[ Sc.Ok ::: Respond '[] RawBody ]

type Endpoint_1 = Endpoint () '[ Method_1, Method_2, Method_3 ]

type Api_1 = Const "users" :> Endpoint_1

type Api_2 = Const "users" :> Seg "user_id" Int :> Endpoint_1

type Api_3 = Wildcard :> Endpoint_1

type Api_All =  Const "api"
             -- :> OneOf '[ Api_1, Api_2, Api_3 ]
             :> OneOf '[ Api_1, Api_3 ]

implAll :: Impl IO Api_All
implAll = impl_1 :<|> impl_3 :<|> NotFound
--implAll = impl_1 :<|> impl_2 :<|> impl_3 :<|> NotFound
  where
    impl_1 = delete :<|> post :<|> get :<|> MethodNotAllowed
    impl_2 _userId = delete :<|> post :<|> get :<|> MethodNotAllowed
    impl_3 _wildcard = delete :<|> post :<|> get :<|> MethodNotAllowed

    delete =
      respond 
      $ emptyResponse Sc.SNoContent 
    post = 
      respond
      $ emptyResponse Sc.SNoContent
      & withHeader H.SLocation "example.com"
    get = 
      respond
      $ emptyResponse Sc.SOk
      & withBody "hello"

apiProxy :: Sing Api_All
apiProxy = sing

apiServer :: Server IO
apiServer = server apiProxy implAll

application :: Wai.Application
application = makeApplication defaultConfig apiServer

main :: IO ()
main = putStrLn "hello world"

Note the commented out lines. If I uncomment them, I get the following error:

      /home/aspidites/.stack/setup-exe-cache/x86_64-linux/setup-Simple-Cabal-1.22.5.0-ghc-7.10.3 --builddir=.stack-work/dist/x86_64-linux/Cabal-1.22.5.0 build exe:learn-serv --ghc-options " -ddump-hi -ddump-to-file"
    Process exited with code: ExitFailure 1
    Logs have been written to: /home/aspidites/projects/learn-serv/.stack-work/logs/learn-serv-0.1.0.0.log

Configuring learn-serv-0.1.0.0...
Preprocessing executable 'learn-serv' for learn-serv-0.1.0.0...
[1 of 1] Compiling Main ( src/Main.hs, .stack-work/dist/x86_64-linux/Cabal-1.22.5.0/build/learn-serv/learn-serv-tmp/Main.o )

/home/aspidites/projects/learn-serv/src/Main.hs:63:13:
No instance for (Serv.Internal.URI.URIDecode Int)
arising from a use of ‘server’
In the expression: server apiProxy implAll
In an equation for ‘apiServer’: apiServer = server apiProxy implAll


The error somewhat makes since in that one of the endpoints depends on an integer capture, but I don't understand what server is expecting me to do about it.

That said, this is the last little code puzzle I need to solve before I can actually start rewriting the relevant parts of the tutorial.

@AphonicChaos
Copy link
Contributor Author

I wonder if this is related at all?

@tel
Copy link
Owner

tel commented Feb 8, 2016

That's definitely a shortcoming, but more immediately there aren't that
many instances of URIDecode yet. You could write one. That's also a great
PR :)

https://github.com/tel/serv/blob/master/src/Serv/Internal/URI.hs

On Sun, Feb 7, 2016 at 8:47 PM Edwin Marshall notifications@github.com
wrote:

I wonder if this
https://github.com/tel/serv/blob/master/src/Serv/Internal/Server.hs#L190
is related at all?


Reply to this email directly or view it on GitHub
#10 (comment).

@AphonicChaos
Copy link
Contributor Author

I guess what I'm more interested in for this PR is if I'm doing something wrong which is requiring this instance, or if solving my last puzzle indeed requires writing an instance?

:<|> IO (Response '['H.Location '::: RawText] 'Empty)
:<|> IO (Response '[] ('Body '[Ct.TextPlain] Text))
:<|> IO NotHere
type Impl_Endpoint_ =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should I do here? I don't need Impl_Endpoint_, but I'm not sure what the preceding paragraph should be changed to, as there is no longer a need to refactor. Can I simply get rid of everything from ~194 to ~246?

Copy link
Owner

Choose a reason for hiding this comment

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

That seems plausible. We can restore that bit if we need it, but things have changed there just a bit.

@tel tel added the docs label Feb 8, 2016
@AphonicChaos
Copy link
Contributor Author

@tel I spent about an hour updating the readme and following along with a test project as I went. Everything compiles, but I'm not sure if all the sections make sense. As I am still a bit hairy on how everything works, I'd appreciate recommendations for how to update certain bits of text. As you're the main author of the library, I suspect that you will be better able to convey how the individual pieces come together. I'm particular concerned when it comes to my changes around lines ~243-256.

I probably won't get a chance to work on this again until mid next week. I'll be on vacation then, though I'll also be going to a lot of doctor's appointments.

@tel
Copy link
Owner

tel commented Feb 11, 2016

I'm beginning to think that a good approach to taking aim at the README is to expand the Wiki with useful information and then we'll patch together the right bits of that for the README. Perhaps a good way to take the work you've been doing is to go make a Wiki page to document and explore these examples?

@AphonicChaos
Copy link
Contributor Author

Frankly, I started this by wanting to fix a typo as I did some exploration. I had no intent in investing so much time on a library that, honestly, I'm not committed to. Feel free to do what you will with what I've done so far, but I'm not at all interested in converting this to a wiki article.

@tel
Copy link
Owner

tel commented Feb 11, 2016

Ah, sorry to hear that. To be clear, I appreciate the time you've spent investigating and don't mean to imply one thing or another about it. Whatever contribution you feel like preparing is wonderful and if you don't feel it worth the time to contribute at all then I still appreciate your interest.

If you're unlikely to continue this then I may close this PR. Please let me know what you think is best.

@AphonicChaos
Copy link
Contributor Author

If you intend to move to more detailed wiki information, it's probably best to just close this PR. I'll probably keep an eye out on this library's progress as it was interesting to mess with, but feels a bit too foreign to me at my current proficiency level.

Thanks for your time.

@tel
Copy link
Owner

tel commented Feb 11, 2016

Again, thank you!

Right now I'm happy (well, not really) to admit there's a lot of mess around here, but I'm eager to get it to a point where it's more intelligible and useful. I'll also probably make some materials to explain the innards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants