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

Description and Summary combinators #767

Merged
merged 5 commits into from Aug 16, 2017

Conversation

Projects
None yet
5 participants
@fierce-katie
Copy link
Contributor

fierce-katie commented Jun 8, 2017

This PR adds combinators discussed in haskell-servant/servant-swagger#60. They allow you to add documentation for API endpoints right in the type declaration.

Both Description and Summary are needed for different HasSwagger instances:

instance (KnownSymbol desc, HasSwagger api) => HasSwagger (Description desc :> api) where
  toSwagger _ = toSwagger (Proxy @api)
    & allOperations.description %~ (Just (Text.pack (symbolVal (Proxy @desc))) <>)

instance (KnownSymbol desc, HasSwagger api) => HasSwagger (Summary desc :> api) where
  toSwagger _ = toSwagger (Proxy @api)
    & allOperations.summary %~ (Just (Text.pack (symbolVal (Proxy @desc))) <>)
@fizruk

This comment has been minimized.

Copy link
Member

fizruk commented Jun 14, 2017

Build fails for GHC 7.8 with

    /home/travis/build/haskell-servant/servant/servant-server/test/Servant/ServerSpec.hs:71:5:
        Context reduction stack overflow; size = 21
        Use -fcontext-stack=N to increase stack size to N
          Servant.Server.Internal.HasServer
            (Servant.API.Description.Description "foo" :> GET)
            '[NamedContext "foo" '[]]
        In the expression:
          serveWithContext comprehensiveAPI comprehensiveApiContext
        In a pattern binding:
          _ = serveWithContext comprehensiveAPI comprehensiveApiContext

@phadej should we increase stack size for the build?
I guess that error is because of ComprehensiveAPI being too comprehensive for GHC 7.8 :)

@fizruk fizruk requested a review from phadej Jun 14, 2017

@fizruk

fizruk approved these changes Jun 14, 2017

@@ -350,6 +350,20 @@ instance HasForeign lang ftype api
foreignFor lang ftype Proxy req =
foreignFor lang ftype (Proxy :: Proxy api) req

instance HasForeign lang ftype api

This comment has been minimized.

Copy link
@fizruk

fizruk Jun 14, 2017

Member

Here we ignore Summary and Description, but it would be super nice to adjust servant-foreign so that we could save descriptions to later translate them into comments.

This comment has been minimized.

Copy link
@fierce-katie

fierce-katie Jun 14, 2017

Author Contributor

Opened #763 for it.

@@ -533,6 +534,18 @@ instance HasServer api context => HasServer (HttpVersion :> api) context where
route Proxy context subserver =
route (Proxy :: Proxy api) context (passToServer subserver httpVersion)

-- | Ignore @'Summary'@ in server handlers.

This comment has been minimized.

Copy link
@fizruk

fizruk Jun 14, 2017

Member

Can/should we use summary/description in error messages?

This comment has been minimized.

Copy link
@phadej

phadej Jun 14, 2017

Member

I have no idea how the implementation would look like...

This comment has been minimized.

Copy link
@fizruk

fizruk Jun 14, 2017

Member

We could (optionally) inject description/summary for 4xx (client side) errors.

This comment has been minimized.

Copy link
@phadej

phadej Jun 14, 2017

Member

Yeah I understand. We'd need some kind of mapError post phase in Delayed to do that. Can you or @fierce-katie open a ticket about that. Let's not complicate this PR

This comment has been minimized.

Copy link
@fizruk

fizruk Jun 14, 2017

Member

Created #768.


import Data.Typeable (Typeable)
import GHC.TypeLits (Symbol)
-- | Add a short summary for (part of) API.

This comment has been minimized.

Copy link
@phadej

phadej Jun 14, 2017

Member

could we call it Synopsis? Would be more in line with Cabal terminology :P

This comment has been minimized.

Copy link
@fizruk

fizruk Jun 14, 2017

Member

We call it Summary since it's what it's called in Swagger terminology, but I'm ok with Synopsis too :)

This comment has been minimized.

Copy link
@phadej

This comment has been minimized.

Copy link
@alpmestan

alpmestan Jun 14, 2017

Contributor

I don't care, both are fine. =P

-- Example:
--
-- >>> type MyApi = Description "Some longer implementation details here." :> "books" :> Capture "isbn" Text :> Get '[JSON] Book
data Description (sym :: Symbol)

This comment has been minimized.

Copy link
@phadej

phadej Jun 14, 2017

Member

I see the motivation, but is it really practical to write any >=80 char description in a type-level? @fizruk?

This comment has been minimized.

Copy link
@fizruk

fizruk Jun 14, 2017

Member

Why not? :)

-- | This comment is only visible in Haskell sources and Haddocks.
-- It can be really long and is very easy to read.
-- But we can't extract it even with TH :(
type MyEndpoint = Get '[JSON] Resource

type MyEndpoint = Description
  "This comment is visible in multiple Servant interpretations \
  \and can be really long if necessary. \
  \Haskell multiline support is not perfect \
  \but it's still very readable."
 :> Get '[JSON] Resource

This comment has been minimized.

Copy link
@fizruk

fizruk Jun 14, 2017

Member

@fierce-katie this should get into examples/doctests/tutorial to demonstrate multiline descriptions.

This comment has been minimized.

Copy link
@3noch

3noch Jun 14, 2017

Now...if only we had a quasiquoter that would produce Symbols instead of Strings.

@@ -533,6 +534,18 @@ instance HasServer api context => HasServer (HttpVersion :> api) context where
route Proxy context subserver =
route (Proxy :: Proxy api) context (passToServer subserver httpVersion)

-- | Ignore @'Summary'@ in server handlers.

This comment has been minimized.

Copy link
@phadej

phadej Jun 14, 2017

Member

I have no idea how the implementation would look like...

@phadej

This comment has been minimized.

Copy link
Member

phadej commented Jun 14, 2017

I'm 👍 with this. Having two very similar things: Synopsis / Summary and Description is somewhat arbitrary, but I see the motivation, so not against it.

@phadej

This comment has been minimized.

Copy link
Member

phadej commented Jun 14, 2017

about -fcontext-stack=N, yes. please increase it in a spec file, I don't want to have it 0 because we won't catch looping stuff then (as we have UndecidableInstances etc.)

@phadej

This comment has been minimized.

Copy link
Member

phadej commented Jun 19, 2017

@fierce-katie is there something you want to do here, or is this done from your side? I'd like to merge this!

@fierce-katie

This comment has been minimized.

Copy link
Contributor Author

fierce-katie commented Jun 19, 2017

@phadej looks like I've made all the fixes discussed above, so feel free to merge :)

@fierce-katie

This comment has been minimized.

Copy link
Contributor Author

fierce-katie commented Aug 16, 2017

Ping @phadej

@phadej

This comment has been minimized.

Copy link
Member

phadej commented Aug 16, 2017

Totally forgot, thanks for the ping.

@phadej phadej merged commit 50be3a2 into haskell-servant:master Aug 16, 2017

1 check passed

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

@fierce-katie fierce-katie deleted the fierce-katie:docs-combinators branch Aug 16, 2017

@phadej phadej added this to the 0.12 milestone Nov 6, 2017

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.