Skip to content

Conversation

@nsaunders
Copy link
Member

What does this pull request do?

This pull request is a follow-up to #12 as well as purescript-globals#18. With a safe encodeURIComponent function available in purescript-globals as of 4.1.0, we can surface possible failure in this library's encode API by changing its type to FormURLEncoded -> Maybe String.

Where should the reviewer start?

You will find that the code changes involved are minimal.

How should this be manually tested?

I figured that verifying the example shown in the README (including my small update) was a sensible test:

encode $ fromArray [ Tuple "hey" Nothing, Tuple "Oh" $ Just "Let's go!" ]
(Just "hey&Oh=Let's%20go!")

Other Notes:

  • I already updated the README, so no follow-up should be required there.
  • At the risk of stating the obvious, this is a breaking API change.
  • I will update Add decode function. #12 if and when this pull request is merged.

One more thing... Thanks @thomashoneyman and @garyb for your suggestions and for making this possible to implement by updating purescript-globals.

Copy link
Contributor

@thomashoneyman thomashoneyman left a comment

Choose a reason for hiding this comment

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

Thanks @nsaunders! I think it looks good -- just a couple suggestions.

Would you mind updating the README so that it mentions both encoding and decoding now, with examples? With that in place I'd be happy to merge, failing any other suggestions from @garyb.

@thomashoneyman thomashoneyman self-assigned this Aug 12, 2019
@thomashoneyman
Copy link
Contributor

Whoops -- I forgot you added that in #12. Never mind!

@nsaunders
Copy link
Member Author

Great suggestion to use traverse @thomashoneyman, thanks!

@thomashoneyman
Copy link
Contributor

@nsaunders I'm going to wait to see if @garyb has anything else to say about this or #12; if not, then -- if you've updated #12 to use the new safe functions -- tomorrow I'll merge both of them in

@nsaunders
Copy link
Member Author

@thomashoneyman thanks - #12 has been updated as well, but, since it is rebased over this branch, you'll get a better view of the diffs if you merge this one first.

@thomashoneyman thomashoneyman merged commit 0297a0b into purescript-contrib:master Aug 14, 2019
@thomashoneyman
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants