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.

Sign up for GitHub

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

Jump to bottom

Forms #32

Merged
merged 43 commits into from
Aug 31, 2016
Merged

Forms #32

merged 43 commits into from
Aug 31, 2016

Conversation

fizruk
Copy link
Owner

@fizruk fizruk commented Aug 30, 2016

Closes #23. Supersedes #31 and #25.

Some further questions:

  • do we want FormOptions to mimic aeson's Options?
  • do we want ToFormKey/FromFormKey instances for Maybe a, Either a b, First a, Last a like we have for ToHttpApiData and FromHttpApiData?
  • do we want specialised instances for Map Text Text, HashMap Text Text and [(Text, Text)]?
  • do we want to bikeshed functions names (helpers in particular)?

/cc @jkarni @cdepillabout

jkarni and others added 26 commits August 24, 2016 22:23
@jkarni @cdepillabout
Move FormUrlEncoded from servant.
1b468b0 
    This commit moves ToFormUrlEncoded and FromFormUrlEncoded from servant to
    http-api-data. The classes are renamed ToForm and FromForm, and gain
    generic instances.
@cdepillabout
Fix test for decodeForm . encodeForm
0e7f261 
`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
Fix PR so it works on ghc-7.8.
ac72f9e
@cdepillabout
Remove dependency on string-conversions package.
acc2cd4 
This commit replaces all uses of the `cs` function with functions like
`pack`, `unpack`, `decodeUtf8`, etc.
@cdepillabout
Replace uses for toUrlPiece and parseUrlPiece with toQueryParam and p…
97f200f 
…arseQueryParam.
@cdepillabout
Change uses of String to Text.
dd1baa5
@cdepillabout
Add comments and doctests to the FormUrlEncoded module.
8ab1a1f
@cdepillabout
Add two convenience functions for encoding / decoding a ByteString di…
6487f48 
…rectly to / from a datatype.
@fizruk
Rephrase documentation and amend some examples for FormUrlEncoded
fb08da7
@fizruk
Use a custom Show instanse for Form (like Map has)
6ab8a56
@fizruk
Rename encode/decodeWith*From to encode/decodeAsForm
236c59a
@fizruk
Ignore stack haddock's output to src/
7c3d63b
@fizruk
Move Form-related stuff to Web.FormUrlEncoded
4233ec9
@fizruk
Simplify To/FromForm instances for lists
9b7a61d
@fizruk
Remove utf8-string dependency
6e97168
@fizruk
Refactor decodeForm
f2a7554
@fizruk
Remove mtl dependency, simplify GToForm and GFromForm
28713e6
@fizruk
Add test case for empty form data
765249b
@fizruk
Refactor qualified names
d04cfba
@fizruk
Allow duplicate keys in Form
5322053
@fizruk
Add FormKey classes
3d6065a
@fizruk
Generalize ToForm/FromForm instances
a9976c2
@fizruk
Add ToForm/FormForm instances for IntMap and HashMap
6cd7a03
@fizruk
Add helpers for FromForm instances
70060ee
@fizruk
Change Form's internal representation to HashMap
0456604
@fizruk
Export Generic-based toForm and fromForm
550df9a
jkarni
jkarni reviewed Aug 30, 2016
View reviewed changes
src/Web/Internal/FormUrlEncoded.hs

-- | Typeclass for types that can be parsed from keys of a 'Form'. This is the reverse of 'ToFormKey'.
class FromFormKey k where
-- | Parse a key of a 'Form'.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the W3 docs, it seems keys (control names, in the lingo) should be case-insensitive.

https://www.w3.org/TR/html401/interact/forms.html#control-name
https://www.w3.org/TR/html401/interact/forms.html#h-17.3

So maybe this needs to be CI Text?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Interesting, we might want to use CI Text for helpers in Web.HttpApiData too.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@jkarni can't find anywhere that "control names" should be case insensitive.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was looking at name in 17.3, but that's not right. id says it's case sensitive. So I think it's right as it stands.

@fizruk fizruk force-pushed the cdepillabout-add-form-url-encoded branch from 7e55dd8 to 5ae2f86 Compare August 30, 2016 13:27
jkarni
jkarni reviewed Aug 30, 2016
View reviewed changes
src/Web/Internal/FormUrlEncoded.hs
encodeForm :: Form -> BSL.ByteString
encodeForm xs = BSL.intercalate "&" $ map (BSL.fromStrict . encodePair) $ toList xs
where
escape = Text.encodeUtf8 . Text.pack . escapeURIString isUnreserved . Text.unpack
Copy link
Contributor

Choose a reason for hiding this comment

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

This is obviously inefficient. I'm in favor of making changes incrementally, and so leaving the fixing of it for another PR, but for reference [uri-bytestrings](%28https://hackage.haskell.org/package/uri-bytestring-0.2.2.0/docs/URI-ByteString.html%29 could) urlEncode could be a good option.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Aha, I was looking for something like it!
urlEncodeQuery looks ideal for encoding.

@fizruk
Copy link
Owner Author

fizruk commented Aug 30, 2016

@jkarni ah, I still want to remove support for them for now. If there's demand we'll get back to this problem later.

@fizruk fizruk force-pushed the cdepillabout-add-form-url-encoded branch 2 times, most recently from 2fa39c1 to ec3b440 Compare August 30, 2016 21:05
@fizruk fizruk force-pushed the cdepillabout-add-form-url-encoded branch from ec3b440 to e9c728b Compare August 30, 2016 21:12
@cdepillabout cdepillabout mentioned this pull request Aug 31, 2016
Closed
@jkarni jkarni mentioned this pull request Aug 31, 2016
Closed
@cdepillabout
Copy link
Contributor

@fizruk: This looks good to me.

One thing I'd like to see is an example using FormOptions. One thing that always bugs me is when I look at lens or aeson documentation is that there is not enough explanation for using their generic deriving options.

For example, take a datatype like this:

data Person = Person
  { personName :: Text
  , personAge :: Int
  }

I imagine it's common to want to urlencode this to be name=foo&age=30, not personName=foo&personAge=30.

An example of how to use defaultOptions to accomplish this would be really nice.

Of course, I can always send a PR for this additional documentation after this PR (#32) has been merged.

@fizruk fizruk force-pushed the cdepillabout-add-form-url-encoded branch from 570353a to 9cb90d5 Compare August 31, 2016 00:27
@fizruk fizruk force-pushed the cdepillabout-add-form-url-encoded branch from acae0c8 to de9fe0f Compare August 31, 2016 00:54
@fizruk
Copy link
Owner Author

fizruk commented Aug 31, 2016

@cdepillabout added an example in e613d05.

Any final wishes before I release? :)

@jkarni
Copy link
Contributor

jkarni commented Aug 31, 2016

I wish you a good flight?

@fizruk
Copy link
Owner Author

fizruk commented Aug 31, 2016

@jkarni yeah, thanks :)

I am merging this now, I think we've covered most things here.

@fizruk fizruk merged commit 56c53a4 into master Aug 31, 2016
@fizruk fizruk deleted the cdepillabout-add-form-url-encoded branch August 31, 2016 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fizruk @jkarni @cdepillabout