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

Made json encoding/decoding configurable + Aeson compatible encoding/decoding #12

Closed
wants to merge 16 commits into from

Conversation

eskimor
Copy link

@eskimor eskimor commented Jan 25, 2016

in order to provide an aeson compatible encoding/decoding option. Also resolving issue #5.

Needs more testing & cleanup before being ready for a PR
Tests for existing functionality passing. Tests for aeson encoding
missing - coming soon.
@garyb
Copy link
Member

garyb commented Jan 25, 2016

@hdgarrood could you take a look at this perhaps, if you have time? You're definitely more familiar with Aeson than me. 😄



allConstructorsNullary :: Array DataConstructor -> Boolean
allConstructorsNullary constrSigns = foldr (&&) true <<< map (\c -> null c.sigValues) $ constrSigns
Copy link
Contributor

Choose a reason for hiding this comment

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

Is foldr (&&) true the same as Data.Foldable.all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, yeah, I think this could be written allConstructorsNullary = Data.Foldable.all (null <<< _.sigValues)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah - I missed Data.Foldable.all during my Pursuit search. Thanks!

@eskimor
Copy link
Author

eskimor commented Jan 25, 2016

I tried to make it backwards compatible. So gEncodeJson became a simple wrapper around genericEncodeJson which takes an Options parameter resulting in the same format previously used.
Breaking backwards compatibility is only, that I dropped gEncodeJson' for obvious reasons, which was exported, but I believe only for testing.

@eskimor
Copy link
Author

eskimor commented Jan 25, 2016

Btw. guys - you are amazing. Sending PRs to purescript projects is so much more fun, than for nixos. Answers in seconds, instead of weeks/months. I don't know how you do that, but it is really motivating.

@hdgarrood
Copy link
Contributor

This looks good to me. Regarding Aeson specifically, I'd just say, if it works, go ahead :)

@hdgarrood
Copy link
Contributor

I do it by happening to be on github at the same time as you, and not having many other commitments right now 😄 it is nice to know that it's appreciated, though. Incidentally, that (not having many commitments) probably won't remain the case for very much longer for me.

@garyb
Copy link
Member

garyb commented Jan 25, 2016

@hdgarrood congrats? 😉

@eskimor I'm in the lucky position to work with PureScript full time now, and especially for libraries like this that we (SlamData) use it means I try to stay on top of things. I don't always manage it though!

@garyb
Copy link
Member

garyb commented Jan 25, 2016

Code looks fine to me too, 👍 but I'd appreciate it if we could have at least a few tests for the Aeson side of things before merging, perhaps just a basic roundtrip encode/decode test for each sig case?

@hdgarrood
Copy link
Contributor

@garyb It's maybe a little early for that, but cheers anyway :)

@eskimor
Copy link
Author

eskimor commented Jan 25, 2016

@garyb : Done :-) I also checked the output - it looks good and aeson compatible.
Is quickCheck generating good test data? It looks too good to be true.

@garyb
Copy link
Member

garyb commented Jan 25, 2016

It depends how good the generators are 😄 but yep, it's wonderful for this kind of testing!

Thanks 👍

@garyb
Copy link
Member

garyb commented Jan 25, 2016

If you don't mind I'll hold off merging this until we do the big psc 0.8 library update (should be really soon now!), as technically this is a breaking change (dropping the primed exports) so will mean bumping all the dependants too. We can add the unsafeCrashWith cases in then too.

@eskimor
Copy link
Author

eskimor commented Jan 25, 2016

Ok - no problem, I will use my local version in the meantime.

@eskimor
Copy link
Author

eskimor commented Jan 26, 2016

Glad you have not merged it yet - I added two more tests and indeed discovered that it was not quite right yet. Ouch! Ouch! Ouch! It was too good to be true.

@eskimor
Copy link
Author

eskimor commented Jan 26, 2016

Resolved issue #5.

@eskimor
Copy link
Author

eskimor commented Mar 1, 2016

Bumped psc version in package.json to 0.8.0 - incorporated the unsafeCrashWith stuff.

@eskimor
Copy link
Author

eskimor commented Mar 2, 2016

Hi there! Can this be merged now? Or is something still missing?

@garyb
Copy link
Member

garyb commented Mar 2, 2016

We haven't finished updating the core libraries for 0.8 yet. I was intending to wait until then, as we'll need to bump all the dependency versions again when that's finished, which will be classed as another breaking change.

@eskimor
Copy link
Author

eskimor commented Mar 3, 2016

Ah I get it now, you would release a new version of purescript-argonaut-codecs too when merging? So master and the latest version always stay the same? I see, ping me if anymore changes are needed for merging.

@eskimor
Copy link
Author

eskimor commented Mar 11, 2016

Resolved issue #14

@eskimor
Copy link
Author

eskimor commented Mar 11, 2016

Huh - what's wrong with the test suite?

Despite what the defaultOptions in aeson suggest, it works as if
unwrapUnaryRecords was set to true.
@eskimor eskimor changed the title Made json encoding/decoding configurable Made json encoding/decoding configurable + Aeson compatible encoding/decoding Mar 16, 2016
@eskimor
Copy link
Author

eskimor commented Jun 16, 2016

Creating standalone package.

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.

3 participants