Skip to content

Description and Summary combinators#767

Merged
phadej merged 5 commits intohaskell-servant:masterfrom
fierce-katie:docs-combinators
Aug 16, 2017
Merged

Description and Summary combinators#767
phadej merged 5 commits intohaskell-servant:masterfrom
fierce-katie:docs-combinators

Conversation

@fierce-katie
Copy link
Copy Markdown
Contributor

@fierce-katie 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
Copy link
Copy Markdown
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 June 14, 2017 09:07
fizruk
fizruk approved these changes Jun 14, 2017
View reviewed changes
Comment thread servant-foreign/src/Servant/Foreign/Internal.hs
foreignFor lang ftype Proxy req =
foreignFor lang ftype (Proxy :: Proxy api) req

instance HasForeign lang ftype api
Copy link
Copy Markdown
Member

@fizruk fizruk Jun 14, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@fierce-katie fierce-katie Jun 14, 2017

Choose a reason for hiding this comment

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

Opened #763 for it.

Comment thread servant-server/src/Servant/Server/Internal.hs
route Proxy context subserver =
route (Proxy :: Proxy api) context (passToServer subserver httpVersion)

-- | Ignore @'Summary'@ in server handlers.
Copy link
Copy Markdown
Member

@fizruk fizruk Jun 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@phadej phadej Jun 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@fizruk fizruk Jun 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@phadej phadej Jun 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@fizruk fizruk Jun 14, 2017

Choose a reason for hiding this comment

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

Created #768.

phadej
phadej reviewed Jun 14, 2017
View reviewed changes
Comment thread servant/src/Servant/API/Description.hs

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

@phadej phadej Jun 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@fizruk fizruk Jun 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@phadej phadej Jun 14, 2017

Choose a reason for hiding this comment

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

@haskell-servant/maintainers bikeshed please!

Copy link
Copy Markdown
Contributor

@alpmestan alpmestan Jun 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Comment thread servant/src/Servant/API/Description.hs
-- Example:
--
-- >>> type MyApi = Description "Some longer implementation details here." :> "books" :> Capture "isbn" Text :> Get '[JSON] Book
data Description (sym :: Symbol)
Copy link
Copy Markdown
Contributor

@phadej phadej Jun 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@fizruk fizruk Jun 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@fizruk fizruk Jun 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@3noch 3noch Jun 14, 2017

Choose a reason for hiding this comment

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

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

Comment thread servant-server/src/Servant/Server/Internal.hs
route Proxy context subserver =
route (Proxy :: Proxy api) context (passToServer subserver httpVersion)

-- | Ignore @'Summary'@ in server handlers.
Copy link
Copy Markdown
Contributor

@phadej phadej Jun 14, 2017

Choose a reason for hiding this comment

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

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

@phadej
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor

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.)

@fizruk fizruk mentioned this pull request Jun 14, 2017
Open
@phadej
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Contributor Author

fierce-katie commented Aug 16, 2017

Ping @phadej

@phadej
Copy link
Copy Markdown
Contributor

phadej commented Aug 16, 2017

Totally forgot, thanks for the ping.

@phadej phadej merged commit 50be3a2 into haskell-servant:master Aug 16, 2017
@fierce-katie fierce-katie deleted the docs-combinators branch August 16, 2017 12:34
@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

Reviewers

@alpmestan alpmestan alpmestan left review comments

@fizruk fizruk fizruk approved these changes

+2 more reviewers

@phadej phadej phadej left review comments

@3noch 3noch 3noch left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.12

Development

Successfully merging this pull request may close these issues.

5 participants

@fierce-katie @fizruk @phadej @alpmestan @3noch