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

Servant.API.Modifiers #873

Merged
merged 1 commit into from Jan 25, 2018

Conversation

Projects
None yet
4 participants
@phadej
Copy link
Member

phadej commented Dec 10, 2017

Implements modifiers #856.

Combinators

  • QueryParam
  • Header
  • ReqBody

QueryParams? Required -> NonEmpty a?

Implementations

  • servant (Links)
  • servant-client-core
  • servant-server
  • servant-docs
  • servant-foreign
  • Amend ComprehensiveAPI

QA

  • Capture can also be lenient, but I'll leave it for now, but by definition they are always required.

There's some info in the commit messages.

Resolves

  • #862
  • When ReqBody is amended, will solve #793
  • Supersedes #256

@phadej phadej changed the title Add Servant.API.Modifiers to servant Servant.API.Modifiers Dec 10, 2017

@phadej phadej force-pushed the phadej:modifiers branch from 88854a8 to 1205fe6 Dec 10, 2017

@phadej phadej requested a review from alpmestan Dec 10, 2017

@phadej phadej force-pushed the phadej:modifiers branch from 1205fe6 to c453ce0 Dec 10, 2017

@phadej phadej requested a review from fizruk Dec 10, 2017

import qualified Data.Text as Text
import qualified Data.Text.Encoding as TE
import Data.Type.Bool (If)

This comment has been minimized.

Copy link
@jkarni

jkarni Dec 10, 2017

Member

Alignment

type MkLink (HttpVersion:> sub) = MkLink sub
toLink _ = toLink (Proxy :: Proxy sub)

-- | /Note:/ the 'IsSecure' isn't visible in the resulting 'Link'.

This comment has been minimized.

Copy link
@jkarni

jkarni Dec 10, 2017

Member

I don't entirely understand this note.

This comment has been minimized.

Copy link
@phadej

phadej Dec 11, 2017

Author Member

linkURI :: Link -> URI should reflect the scheme, we could use IsSecure combinator for links too, f IsSecure = "https"; f NotSecure = "http".


it "can generate all inks for ComprehensiveAPIWithoutRaw" $ do

This comment has been minimized.

Copy link
@jkarni

jkarni Dec 10, 2017

Member

typo ("links")

-- FoldRequired '[] :: Bool
-- = 'False
--
type FoldRequired mods = FoldRequired' 'False mods

This comment has been minimized.

Copy link
@jkarni

jkarni Dec 10, 2017

Member

Hmm, so here, another option is to have a map:

[IsRequired := 'True, Description := "something"]

(And optionally, type Required = IsRequired := 'True ; type Optional = IsRequired := 'False)

Which simplifies the implementation here, insofar as instead of having separate FoldXs, you can just have a generic "Lookup".

This comment has been minimized.

Copy link
@phadej

phadej Dec 11, 2017

Author Member

Using map-like is an option.

TL;DR, current approach:

  • pros: there isn't (not caught) ill-kinded situations like IsRequired := "true"
  • cons: slight boilerplate in the implementation (not use!)

IMHO pros outweight cons here.


I personally dislike "dynamic" feeling of (:=) :: * -> k -> *. Nothing really prevents you from saying '[IsRequired := "true"]. OTOH, having typed labels is a great improvements over (:=) :: Symbol -> k -> *, proposed earlier, as then typos in label name would get unnoticed.

Think about term level, we could have two APIs:

defaultOptions &
  & required .~ True
  & description ?~ "Foo"
-- or
required <> description "Foo"

The type-level list approach is like the Monoid one.
Having all "type-safety" of lens one (value type is dependent on the key!) is tricky.
My gut feeling that in GHC >= 8.0 one could encode that, but IMHO it's not worth that.

-> RequiredArgument mods a
-> r
foldRequiredArgument _ f g mx =
case (sbool :: SBool (FoldRequired mods), mx) of

This comment has been minimized.

Copy link
@jkarni

jkarni Dec 10, 2017

Member

Have you thought of making SBool an instance of Reifies (from reflection)? It seems like we might want non-boolean as well (e.g. Description), and then using a unified interface would be nice (rather than the one-off SBoolI class)

This comment has been minimized.

Copy link
@phadej

phadej Dec 11, 2017

Author Member

reflection isn't powerful enough. We need something to pattern match on (SingI from singletons, which I'd like to avoid for now as a dependency) to force RequiredArgument mods a into either a or Maybe a.

Note: the instance would be

instance SBoolI b => Reifies Bool Bool where
    reflect _ = case sbool :: ... of STrue -> True; SFalse -> False
| MissingHeader
| UndecodableHeader ByteString
deriving (Typeable, Eq, Show, Functor)
type Header = Header' '[]

This comment has been minimized.

Copy link
@jkarni

jkarni Dec 10, 2017

Member

For these, should we maybe be explicit about Optional etc? That might serve as a little bit of documentation.

This comment has been minimized.

Copy link
@alpmestan

alpmestan Dec 10, 2017

Contributor

+1 on that.

This comment has been minimized.

Copy link
@phadej

phadej Dec 11, 2017

Author Member

Will do.

@alpmestan
Copy link
Contributor

alpmestan left a comment

I'm pretty happy with this. Of course we'll just want some more haddocks and possibly a tutorial update, so that users can easily discover all the modifiers, their effects and how they should be used. Not necessarily in this PR but before this is released.

Servant.API.RemoteHost
Servant.API.ReqBody
Servant.API.ResponseHeaders
Servant.API.Stream

This comment has been minimized.

Copy link
@alpmestan

alpmestan Dec 10, 2017

Contributor

Oh, nice one. I'm feeling better already =P

@alpmestan

This comment has been minimized.

Copy link
Contributor

alpmestan commented Dec 15, 2017

@taktoa I believe you were asking about this on #nixos-dev yesterday. Didn't find you on IRC so I'm pinging you here: this is the upcoming solution to the problem you were asking about.

@taktoa

This comment has been minimized.

Copy link

taktoa commented Dec 15, 2017

@alpmestan Yup, I actually linked this in there. I was mainly asking because I thought someone might have an out-of-tree version of the code that I could use more easily than this patch, but I realized that I don't have a near-term need for the HasServer functionality, so I can just use the version that only has HasClient until this gets merged and upstreamed to Hackage.

@phadej phadej force-pushed the phadej:modifiers branch from f2c9c85 to 062e401 Jan 22, 2018

@phadej phadej added this to the 0.13 milestone Jan 24, 2018

{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE TypeOperators #-}
#if __GLASGOW__HASKELL < 709
{-# OPTIONS_GHC -fcontext-stack=41 #-}

This comment has been minimized.

Copy link
@alpmestan

alpmestan Jan 24, 2018

Contributor

👍

@phadej

This comment has been minimized.

Copy link
Member Author

phadej commented Jan 24, 2018

Green. I'm positively surprised that servant-js isn't broken. I'll reread what I wrote tomorrow, and then merge.

Add Servant.API.Modifiers to servant
Changes Header, ReqBody and QueryParam to take a modifier list.

Resolves #856

ResponseHeader story turns to be somewhat ugly, but it can be made
elegant when #841 is
implemnted, then we can omit HList aka Header Heterogenous List
implementation.

- servant-server changes:

  Writing server side intepretations is quite simple using
  `unfoldRequestArgument`, which makes Header and QueryParam look quite
  the same.

  `ReqBody` cannot be easily made optional with current design (what that
  would mean: No Content-Type Header?), so that dimensions isn't used
  there.

- Add HasLink for all the rest ComprehensiveAPI combinators
- Add 'tricky' Header', QueryParam' endpoints to ComprehensiveAPI
- servant-docs: Quick'n'dirty implementation. Don't use modifiers information (yet).

@phadej phadej force-pushed the phadej:modifiers branch from f5fa422 to bc3f61d Jan 25, 2018

@phadej phadej merged commit bf289cc into haskell-servant:master Jan 25, 2018

1 check passed

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

@phadej phadej deleted the phadej:modifiers branch Jan 25, 2018

@jkarni jkarni referenced this pull request Mar 6, 2018

Closed

Optional request body #793

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.