Skip to content

Conversation

andrewthad
Copy link
Contributor

This PR was formed by:

I would appreciate any feedback if anyone sees anything that looks wrong. In particular, it's possible that I ended up with a non-list container that serializes to a json string instead of an array. All existing tests are still passing.

{-# INLINE list #-}

listValue :: (a -> Value) -> [a] -> Value
listValue f = Array . V.fromList . map f
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps list should be renamed to listEncoding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I had only left it named list here because I stole it out of Data.Aeson.Encode.Functions, but listEncoding is a much better name.

@andrewthad
Copy link
Contributor Author

Pushed some more changes that incorporate the suggestions from @RyanGlScott.

@bergmark bergmark mentioned this pull request May 6, 2016
18 tasks
@bergmark bergmark added this to the 0.12 milestone May 6, 2016
@phadej
Copy link
Collaborator

phadej commented Jun 1, 2016

@andrewthad could you rebase it. Can you write lifted instances for Map and HashMap also, or should I continue on this. I have few cycles this Thursday.

@andrewthad
Copy link
Contributor Author

@phadej I've merged changes from master into this branch. I've also added the higher kinded instances for Map and HashMap. Feel free to take over this branch and add any more instances. I won't have any time to look at it for the next few days.

Also, there's one thing that could use some review. I left the ToJSON and FromJSON instances for Map and HashMap alone instead of redefining them with toJSON1 and friends. It would be better to do it that way, but I have a slight worry that it may hurt performance. If anyone could benchmark it and show that it doesn't hurt performance, it would be great to see the duplication there eliminated.

This was referenced Jun 2, 2016
@andrewthad
Copy link
Contributor Author

Closing in favor of #407

@andrewthad andrewthad closed this Jun 2, 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.

4 participants