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

From/ToFormUrlEncoded (Do not merge) #25

Closed
wants to merge 1 commit into from
Closed

Conversation

jkarni
Copy link
Contributor

@jkarni jkarni commented Jan 22, 2016

Fixes #23. This isn't ready yet. Some questions:

  • It turns out To/FromFormUrlEncoded don't actually expect/return url-encoded forms. Encoding and decoding happens before/after. To me this means their names should be changed to To/FromForm, but I wanted to see what others thought.
  • I imagine using to/parseQueryParam in the generic instance is more correct than to/parseUrlPiece?
  • The generic instance returns the underlying parse error, but not yet the key for which parsing failed. I would say doing that could be done in a separate PR. It involves changing the signature of gFromForm though, and maybe that shouldn't be considered internal.
  • I'm not very familiar with GHC.Generics and generic programming, so there may be issues here.
  • There should probably eventually be an options for the generic stuff.

I still need to check sum record types, squash some commits, and move the tests for encoding/decoding over.

@cdepillabout
Copy link
Contributor

I guess this PR has bee stalled for a while, but is there anything I can do to help push it along?

I've wanted something like this for the last couple months, and I finally created an issue about it over on the servant repository, but I didn't realize it was already being worked on!

    This commit moves ToFormUrlEncoded and FromFormUrlEncoded from servant to
    http-api-data. The classes are renamed ToForm and FromForm, and gain
    generic instances.
@jkarni
Copy link
Contributor Author

jkarni commented Aug 22, 2016

@cdepillabout I just finished up some more tests, squashed, and cleaned up a little. Do you want to try rebasing against master? It's a pretty ugly merge/rebase!

@jkarni
Copy link
Contributor Author

jkarni commented Aug 22, 2016

Relatedly, while moving the encoding and decoding functions over I noticed that they seem to be doing much more converting between String, ByteString and Text than is healthy. At some point we should try to improve on that.

@cdepillabout
Copy link
Contributor

@jkarni Sure, I'll do the rebase onto master. You might have to give a few days though.

@cdepillabout
Copy link
Contributor

@jkarni: I've rebased your code onto master. You can find the updated code here:

#31

@fizruk fizruk mentioned this pull request Aug 30, 2016
@jkarni
Copy link
Contributor Author

jkarni commented Aug 31, 2016

This too has been superseded by #32

@jkarni jkarni closed this Aug 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants