From/ToFormUrlEncoded #31
Conversation
This commit moves ToFormUrlEncoded and FromFormUrlEncoded from servant to
http-api-data. The classes are renamed ToForm and FromForm, and gain
generic instances.
cdepillabout
force-pushed
the
add-form-url-encoded
branch
from
69bec16 to
d9272cd
Compare
`decodeForm (encodeForm x)` does not equal `x` when we have a form like
`Form [("", "")]`. The result `Form` after encoding and decoding is
just `Form []`.
This commit adds in a newtype wrapper `NoEmptyKeyForm` with an
`Arbitrary` instance that removes all empty-string keys.
cdepillabout
force-pushed
the
add-form-url-encoded
branch
from
d9272cd to
ac72f9e
Compare
| import Web.HttpApiData.Internal.HttpApiData | ||
|
|
||
| -- | The contents of a form, not yet url-encoded. | ||
| newtype Form = Form { unForm :: M.Map T.Text T.Text } |
There was a problem hiding this comment.
Is there a reason to use lazy Map and not strict Map or HashMap? /cc @jkarni
|
Hmm, we should not use @cdepillabout will you change every |
| => GFromForm (M1 S sel (K1 i f)) where | ||
| gFromForm f = case M.lookup sel (unForm f) of | ||
| Nothing -> Left . T.unpack $ "Could not find key " <> sel | ||
| Just v -> left T.unpack $ M1 . K1 <$> parseUrlPiece v |
There was a problem hiding this comment.
It will be more appropriate to use parseQueryParam here.
|
@fizruk Thanks for the review. I'll take a crack at fixing these comments. I might need help with some of the documentation-fixes. I'm not very well-versed with |
|
@cdepillabout it's okay, just fix what you are confident in and we'll take care of the rest! |
This commit replaces all uses of the `cs` function with functions like `pack`, `unpack`, `decodeUtf8`, etc.
…rectly to / from a datatype.
cdepillabout
force-pushed
the
add-form-url-encoded
branch
from
b583b37 to
6487f48
Compare
|
@fizruk I fixed up most of your comments. Here's a short description of what changes each commit makes:
decodeWithFromForm :: (FromForm a) => BSL.ByteString -> Either T.Text a
encodeWithToForm :: (ToForm a) => a -> BSL.ByteStringPlease feel free to bikeshed on the names. I don't particularly like |
|
Thanks! I'll amend documentation, fix some other issues and merge later today. |
|
This has been superseded by #32. |
This is PR #25 rebased onto the current master, as requested in #25 (comment).
I've added a commit (0e7f261) that fixes a failing test case. I've also added a commit (ac72f9e) that adds support for ghc-7.8 to the code from @jkarni.